feat(s3): identify Metaflow in the S3 client user agent - #3323
Draft
goanpeca wants to merge 1 commit into
Draft
Conversation
Signed-off-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Type
Summary
S3 clients built by
Boto3ClientProvidernow carry ametaflow/<version>token in theirUser-Agent, so Metaflow traffic is identifiable in server-side access logs. Nothing else about the request changes: no new config, no credential or endpoint handling, no change to the existing retry config block.Issue
No issue yet, which is why this is a draft.
metaflow/plugins/aws/aws_client.pyis Core Runtime, and CONTRIBUTING.md asks for a maintainer-acknowledged issue and an agreed approach before that kind of change. I am opening this to show the concrete 14-line shape of the idea. Happy to file an issue and close this if you prefer that ordering, and equally happy to close it if you do not want the client identifying itself.Reproduction
Not a bug fix, so this is a before/after observation rather than a failing repro.
Runtime: local (client construction only, no requests are sent)
Commands to run:
Where evidence shows up: parent console for the snippet above, and the
User-Agentheader on the wire (server access logs) for real traffic.Before (on master)
After (this branch)
Root Cause
Not a defect, so there is no violated invariant. The gap is that
Boto3ClientProvider.get_clientnever setsuser_agent_extra, so on the wire a Metaflow S3 request is indistinguishable from any other boto3 caller in the same account. That makes it hard for an operator to attribute request volume, throttling, or a cost spike to Metaflow when looking at Amazon S3 server access logs, or at the logs of any other S3-compatible endpoint Metaflow is pointed at (Backblaze B2, Cloudflare R2, MinIO, whichdevtools/already uses for local testing). Most data tools set a client identifier for exactly this reason.botocore'suser_agent_extrais the supported hook for it.Why This Fix Is Correct
The block sits right after the existing retry-config block, reuses the same
client_params.get("config", Config())pattern, and runs only whenmodule == "s3". It appends rather than assigns, so a caller-supplieduser_agent_extrasurvives, and it skips the append when the token is already present, so aConfigobject reused across twoget_clientcalls does not accumulate duplicates.metaflow.versionis imported inside the function, matching how the rest of this function defers its imports.The change is header-only:
user_agent_extrais not part of the signed header set, so signing and presigned URLs are unaffected (verifiedgenerate_presigned_urloutput is identical apart from its expiry timestamp).Failure Modes Considered
Configobject and the dict form ofclient_params["config"]are covered, since the dict is normalized to aConfiga few lines above.Config.user_agent_extrais mutable state on an object the caller owns, so the sameConfigpassed toget_clienttwice would grow a duplicate token. Theua not in existing_ua.split()guard keeps it to one. Token matching is on whitespace-split words sometaflow/2.19.23does not falsely match a longer token that happens to contain it as a substring.non-s3 : None). Keeping the scope narrow means the sandbox STS path and the AWS Batch client behave exactly as before.User-Agentstring, which no Metaflow code reads.Tests
There is no unit test for
Boto3ClientProvidertoday, so this branch adds none. Apytest-mocktest asserting the token lands inclient.meta.config.user_agent_extra, and that a caller-supplied value is preserved rather than replaced, would be straightforward. Tell me if you want it and I will add it here rather than guess at the shape you would accept for a newtest/unit/test_aws_client.py. Manual before/after evidence is above.Non-Goals
No config knob to disable or customize the token. No change to the retry config, endpoint resolution, credential or role-assumption paths, or the sandbox path. No change to non-S3 clients. No change to
metaflow/plugins/datatools/s3/.AI Tool Usage
Claude Code was used to write the patch and to capture the before/after client output above. I reviewed the diff, ran the checks shown here against both
masterand this branch, and can defend the append-not-replace and idempotency choices without going back to the tool.