feat(tracing): respect otel service name via env variable#3231
Conversation
Greptile SummaryThis PR makes Metaflow's OpenTelemetry
Confidence Score: 4/5Safe to merge as a partial improvement — the METAFLOW_OTEL_SERVICE_NAME override works correctly — but the standard OTEL_SERVICE_NAME env var advertised in the PR description is silently ignored. The config lookup uses from_conf("OTEL_SERVICE_NAME", "metaflow"), which internally looks for METAFLOW_OTEL_SERVICE_NAME. The bare OTEL_SERVICE_NAME key — documented as step 2 of the resolution order — is never consulted, so any application that already sets OTEL_SERVICE_NAME to identify itself will not see that value picked up by Metaflow spans. The tests explicitly verify this no-op behavior, confirming it is a real gap rather than a test oversight. metaflow/metaflow_config.py — the from_conf call needs to check os.environ.get("OTEL_SERVICE_NAME") as the default before falling back to "metaflow". Important Files Changed
Reviews (5): Last reviewed commit: "Merge branch 'master' into vivek/otel-se..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Opt-in to inheriting the standard OTEL_SERVICE_NAME env var of the surrounding | ||
| # application. Off by default so upgrades never silently relabel spans for users | ||
| # who already have OTEL_SERVICE_NAME set process-wide. | ||
| OTEL_INHERIT_SERVICE_NAME = from_conf("OTEL_INHERIT_SERVICE_NAME", False) |
There was a problem hiding this comment.
@talsperre -- this is the main area of contention that I'd love your thoughts on! The simpler/cleaner thing here IMO is to just start respecting the OTEL_SERVICE_NAME env var but as Ian rightly pointed out in his issue, that might cause a "breaking change" for existing consumers who might have this variable set in their env but have their o11y/dashboards listening on metaflow. We are in fact, currently relying on metaflow as our service name.
The current approach, explicitly gives them a flag to override the existing behavior and start respecting OTEL_SERVICE_NAME. What do you think?
There was a problem hiding this comment.
I've been mulling over this for a bit now, and I'm wondering whether the env-fallback is a common use case or not. Would love some more insight on peoples use-cases.
The main concern I have here is adding possibly unnecessary/unused options to the Metaflow config. I think we could do without the flag for otel-inheritance as
-
If a user/org wants to use an already configured
OTEL_SERVICE_NAMEin their environment, then they still need to add a new entry to their Metaflow profile to enable this. The edit could just as well be to change the service name through the config at that point. -
If the org wants the ability to set/swap service names as part of their infra instead of Metaflow profiles, and they are managing environment variables separately, then this should only require an additional
METAFLOW_OTEL_SERVICE_NAME=$OTEL_SERVICE_NAMEto the runtime environments in order to override the defaults.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3231 +/- ##
=========================================
Coverage ? 29.88%
=========================================
Files ? 381
Lines ? 52553
Branches ? 9273
=========================================
Hits ? 15704
Misses ? 35673
Partials ? 1176 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
talsperre
left a comment
talsperre
left a comment
There was a problem hiding this comment.
Looks good to me. I reviewed the config resolution, tracer resource wiring, Batch/Kubernetes propagation, and focused unit coverage. The opt-in METAFLOW_OTEL_SERVICE_NAME behavior matches the latest discussion and preserves the default service.name.
3 participants
PR Type
Summary
Make Metaflow's OpenTelemetry
service.nameconfigurable via a newMETAFLOW_OTEL_SERVICE_NAMEconfig instead of hardcoding"metaflow". The default is unchanged ("metaflow"), so this is fully backward compatible.Issue
Fixes #3228
Reproduction
Added a new
OTEL_SERVICE_NAMEconfig inmetaflow_config.py.tracing_modules.pynow readsOTEL_SERVICE_NAMEinstead of the hardcoded"metaflow"string when building the OTelResource.The resolved value is propagated to remote tasks as
METAFLOW_OTEL_SERVICE_NAMEinbatch.pyandkubernetes.py(the K8s job and jobset paths), whereverMETAFLOW_OTEL_ENDPOINTis already forwarded, so remote spans carry the same service name as the client. Argo Workflows and Step Functions are intentionally skipped, as tracing config is not wired through those orchestrators today (would be a larger follow-up).Resolution:
METAFLOW_OTEL_SERVICE_NAMEif set"metaflow"otherwise (unchanged default)Per review discussion, auto-inheriting the bare
OTEL_SERVICE_NAMEenv var was intentionally dropped to avoid silently relabeling spans for users who already setOTEL_SERVICE_NAMEprocess-wide but rely on"metaflow"in their dashboards/alerts. To inherit a surrounding application's service name, setMETAFLOW_OTEL_SERVICE_NAME=$OTEL_SERVICE_NAMEin the runtime environment.Tests
AI Tool Usage