Pass a header into calls to mixer for usage logs#276
Pass a header into calls to mixer for usage logs#276lucyking140 merged 27 commits intodatacommonsorg:masterfrom
Conversation
|
/gcbrun |
hqpho
left a comment
hqpho
left a comment
There was a problem hiding this comment.
In principle, LG! Nitty naming thing: since both "metadata" and "source" are either already or likely to soon become overloaded terms in the DC ecosystem, WDYT of calling the parameter something like surface_header_value or feature_surface_tag or something like that?
datacommons_client/endpoints/base.py
Outdated
| # if this call originates from another DC product (MCP server, DataGemma, etc.), we indicate that to Mixer | ||
| if(metadata_source): | ||
| # makes it clearer to public users that this is a tag specific to other DataCommons features | ||
| if(metadata_source != "mcp" and metadata_source != "datagemma"): |
There was a problem hiding this comment.
Optional: Define a constant array of allowed values somewhere
There was a problem hiding this comment.
Added a list! Also expanded this to include "mcp-{some number}" to support MCP versions like Christie suggested. Also renamed the param to surface_header_value!
|
/gcbrun |
|
/gcbrun |
hqpho
left a comment
hqpho
left a comment
There was a problem hiding this comment.
This change will require a PyPi release to take effect. Let's coordinate that after it's merged.
| Raised when a surface_header_value is passed in that is not a valid Data Commons surface. | ||
| This value is used in the DC usage logs in Mixer | ||
| """ | ||
| default_message = "The surface header value should only to indicate a call made from Data Commons surfaces like the MCP server or DataGemma." |
There was a problem hiding this comment.
Possible edit: "The surface header value must be a surface known to the Data Commons team."
datacommons_client/endpoints/base.py
Outdated
|
|
||
| url = (self.base_url if endpoint is None else f"{self.base_url}/{endpoint}") | ||
| headers = self.headers | ||
| # if this call originates from another DC product (MCP server, DataGemma, etc.), we indicate that to Mixer |
There was a problem hiding this comment.
Optional: mention the default here
There was a problem hiding this comment.
Added! And moved this to the new location where I'm setting the header
datacommons_client/endpoints/base.py
Outdated
| dc_instance = "datacommons.org" | ||
|
|
||
| self.headers = build_headers(api_key) | ||
| self.api_key = api_key if api_key else None |
There was a problem hiding this comment.
Is self.api_key referenced anywhere else? If not, consider passing it as an explicit additional param to build_headers.
There was a problem hiding this comment.
Moved it to a param!
| @patch("requests.post") | ||
| def test_send_post_request_surface_header(mock_post): | ||
| """Tests a successful POST request with a surface header.""" | ||
| mock_response = MagicMock() |
There was a problem hiding this comment.
Nit and usually would be auto-formatted in other repos: I think there needs to be a blank line after the comment
2 participants
clientlib-pythonheader to the newerdatacommons_clientclient librarydatacommonsclient because it makes calls to the V1 API, which will be deprecated soon.surface_header_valuetag in the observation endpoints so that other DC surfaces that make calls to the client library (MCP server, DataGemma, etc.) can be tracked.Tests:
clientlib-pythoncase and when we pass inmetadata_source