-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[DRAFT] fix: deduplicate x-goog-api-client headers #17616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
7658d02
7d266a3
852c5b1
0d8865a
76274ca
e42f0a3
fc05e08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -57,6 +57,35 @@ def _apply_decorators(func, decorators): | |||||
| return func | ||||||
|
|
||||||
|
|
||||||
| def _extract_metrics_header(metadata): | ||||||
| """Extract x-google-api-client header from metadata list. | ||||||
|
|
||||||
| Args: | ||||||
| metadata (Sequence[Tuple[str, str]]): The metadata to extract from. | ||||||
|
|
||||||
| Returns: | ||||||
| Tuple[List[Tuple[str, str]], List[str]]: A tuple containing: | ||||||
| - A list of remaining metadata tuples. | ||||||
| - A list of metrics header values found. | ||||||
| """ | ||||||
| if not metadata: | ||||||
| return [], [] | ||||||
|
|
||||||
| for i, (key, val) in enumerate(metadata): | ||||||
| if key == client_info.METRICS_METADATA_KEY: | ||||||
| # Key located. Check the rest of the list for duplicate entries | ||||||
| arbitrary_metadata = list(metadata[:i]) | ||||||
| metric_values = [val] | ||||||
| for k, v in metadata[i + 1 :]: | ||||||
| if k == client_info.METRICS_METADATA_KEY: | ||||||
| metric_values.append(v) | ||||||
| else: | ||||||
| arbitrary_metadata.append((k, v)) | ||||||
| return arbitrary_metadata, metric_values | ||||||
| # No key found | ||||||
| return list(metadata), [] | ||||||
|
|
||||||
|
|
||||||
| class _GapicCallable(object): | ||||||
| """Callable that applies retry, timeout, and metadata logic. | ||||||
|
|
||||||
|
|
@@ -91,6 +120,9 @@ def __init__( | |||||
| self._timeout = timeout | ||||||
| self._compression = compression | ||||||
| self._metadata = metadata | ||||||
| # Pre-extract the x-goog-api-client header from the initialized metadata. | ||||||
| self._arbitrary_metadata, metric_values = _extract_metrics_header(metadata) | ||||||
| self._metrics_values = " ".join(metric_values) if metric_values else "" | ||||||
|
|
||||||
| def __call__( | ||||||
| self, *args, timeout=DEFAULT, retry=DEFAULT, compression=DEFAULT, **kwargs | ||||||
|
|
@@ -114,14 +146,26 @@ def __call__( | |||||
|
|
||||||
| # Add the user agent metadata to the call. | ||||||
| if self._metadata is not None: | ||||||
| metadata = kwargs.get("metadata", []) | ||||||
| # Due to the nature of invocation, None should be treated the same | ||||||
| # as not specified. | ||||||
| if metadata is None: | ||||||
| metadata = [] | ||||||
| metadata = list(metadata) | ||||||
| metadata.extend(self._metadata) | ||||||
| kwargs["metadata"] = metadata | ||||||
| metadata = kwargs.get("metadata") | ||||||
| if not metadata: | ||||||
| if self._metrics_values: | ||||||
| kwargs["metadata"] = [ | ||||||
| (client_info.METRICS_METADATA_KEY, self._metrics_values) | ||||||
| ] + self._arbitrary_metadata | ||||||
| else: | ||||||
| kwargs["metadata"] = self._arbitrary_metadata | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assigning To prevent shared state mutation, please copy the list using
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please address this @daniel-sanche
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll have to check the overhead on the copy operation. Maybe we should make self._arbitrary_metadata an immutable tuple instead. I will take another look when I look at the code next |
||||||
| else: | ||||||
| # Merge user-supplied metadata with library-supplied metadata. | ||||||
| merged_metadata, api_client_values = _extract_metrics_header(metadata) | ||||||
| if self._metrics_values: | ||||||
| api_client_values.append(self._metrics_values) | ||||||
| if api_client_values: | ||||||
| merged_metadata.append( | ||||||
| (client_info.METRICS_METADATA_KEY, " ".join(api_client_values)) | ||||||
| ) | ||||||
| merged_metadata.extend(self._arbitrary_metadata) | ||||||
| kwargs["metadata"] = merged_metadata | ||||||
|
|
||||||
| if self._compression is not None: | ||||||
| kwargs["compression"] = compression | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of
_extract_metrics_headeruses index slicing (metadata[:i]andmetadata[i+1:]) and multiple loops. This can be simplified into a single-pass loop that is more readable, performs better, and is more robust because it works on any iterable (including generators/iterators) without raising aTypeErroron slicing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Gemini's comment.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an attempted optimization to avoid re-building the metadata list from scratch each time. This is a critical part of the code that is executed before every single request, and we expect the vast majority of requests to not contain custom metadata, so if there's any way to make a fast-path here, we should use it.
I haven't done bench-marking on this yet though, so I'm not sure if this specific approach is actually any better. Looking again, the list conversion at the end could end up negating any benefits. But we should experiment with this and do some quick validation before merging anything, since this is such a critical part of the libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, thanks giving the context. I'll leave it upto you what you decide based on benchmarking.