fix(sdk): regenerate service.instance.id post-fork in MeterProvider and TracerProvider#5000
fix(sdk): regenerate service.instance.id post-fork in MeterProvider and TracerProvider#5000sterchelen wants to merge 3 commits into
Conversation
| This hook runs post-fork in each worker and replaces service.instance.id | ||
| with a fresh UUID, ensuring each worker is a distinct instance. | ||
| """ | ||
| self._sdk_config.resource = self._sdk_config.resource.merge( |
There was a problem hiding this comment.
We are not setting this in the resource attributes, also not sure the issue is metrics specific.
There was a problem hiding this comment.
We are not setting this in the resource attributes
Not sure to get your point but if the user didn't set it, workers would be completely indistinguishable in the backend after fork, which is worse than having no ID at all. By always generating one post-fork we ensure every worker has a distinct identity regardless of whether the user configured it upfront.
On the second point: agreed, the problem affects both metrics and traces 👍🏼
There was a problem hiding this comment.
My point is that if we have a solution that work fine after forks() since the semantic conventions is now stable we can add this to the default resource detector. This way we have the very same attributes before and after fork.
sterchelen
force-pushed
the
fix/post-fork-service-instance-id
branch
2 times, most recently
from
63676db to
70b6fff
Compare
The Python SDK did not auto-generate service.instance.id, unlike the Java SDK and the stable semantic convention recommendation. Add it to _DEFAULT_RESOURCE so every process gets a unique instance identity at startup without any user configuration.
…nd TracerProvider When a prefork server (e.g. gunicorn) forks workers, all workers inherit the same Resource from the master process, including the same service.instance.id. Register an os.register_at_fork(after_in_child=...) hook on both MeterProvider and TracerProvider that replaces service.instance.id with a fresh UUID in each forked worker, ensuring distinct resource identities without any user configuration. Resource.merge() preserves all other resource attributes. WeakMethod is used for the hook reference, consistent with the existing pattern in PeriodicExportingMetricReader and BatchSpanProcessor. Fixes: open-telemetry#4390 Related: open-telemetry#3885
sterchelen
force-pushed
the
fix/post-fork-service-instance-id
branch
from
70b6fff to
3256bf0
Compare
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
No I am still waiting for review :) |
There was a problem hiding this comment.
@sterchelen Thanks for the PR and sorry for the late review on this. I was dealing with that issue recently and just saw your PR today
I believe we need a single shared fork hook and a provider registry so the child process generates one UUID, and you iterate over the providers set in the registry and apply the new UUID to all registered providers within the forked process.
My understanding is that a provider created after the fork should also get the same child UUID when it is created.
| with a fresh UUID, ensuring each worker is a distinct instance. | ||
| """ | ||
| self._sdk_config.resource = self._sdk_config.resource.merge( | ||
| Resource({"service.instance.id": str(uuid4())}) |
There was a problem hiding this comment.
This will generate a unique id for metrics and another id for traces. My understanding is that this id should be the same for both providers, but still unique at the instance level.
There was a problem hiding this comment.
You are right (and sorry for my late answer... dealing with a complex migration).
I'll do that.
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
This PR has been closed due to inactivity. Please reopen if you would like to continue working on it. |
github-project-automation
Bot
moved this from Reviewed PRs that need fixes
to Done
in Python PR digest
|
I can't reopen my PR ... |
Address review on open-telemetry#5000: the per-provider fork hook generated a separate uuid4() for MeterProvider and TracerProvider, so after a fork metrics and traces in the same worker reported different service.instance.id values. Introduce a shared registry and a single os.register_at_fork hook in opentelemetry.sdk._shared_internal._fork. The child generates one UUID and applies it to every registered provider; providers created after the fork adopt the same id at construction via register_provider(). A threading.Lock guards the registry (thread-safe registration, free-threading ready) and is acquired in the before hook / released in after_in_parent/after_in_child to stay fork-safe. Providers now expose _reset_service_instance_id() instead of _at_fork_reinit(). Fixes: open-telemetry#4390 Related: open-telemetry#3885
|
@emdneto, could you reopen the PR for me please? I pushed the change you requested 🙏🏼 |
|
@sterchelen just reopened the thread. Will take a look at your changes. Thanks |
|
@emdneto thx ; I answered in the conversation you shared to this PR 👍🏼 |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
Hey @sterchelen I think #5280 will supersede this PR once merged. |
|
Closing in favor of #5280 |
3 participants
Description
When a prefork server (e.g. gunicorn) forks workers, all workers inherit the same
Resourcefrom the master process — including the sameservice.instance.id. The SDK already restarts background threads post-fork (PeriodicExportingMetricReader,BatchSpanProcessor) but never updates the resource identity. This causes metric and trace collisions in OTLP backends where multiple workers exporting with the same resource identity result in incorrect aggregation (last-write-wins) instead of correct summation.This PR registers an
os.register_at_fork(after_in_child=...)hook on bothMeterProviderandTracerProviderthat replacesservice.instance.idwith a fresh UUID in each forked worker. All other resource attributes are preserved viaResource.merge().WeakMethodis used for the hook reference, consistent with the existing pattern inPeriodicExportingMetricReaderandBatchSpanProcessor.Fixes #4390
Related: #3885
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Two new test files covering both providers:
tests/metrics/test_meter_provider_fork.pytests/trace/test_tracer_provider_fork.pyEach covers:
_at_fork_reinitassigns a newservice.instance.idOther resource attributes are preserved
A real fork() produces a distinct ID in the child vs the parent
4 concurrent forks each produce a unique ID
Run with:
pytest opentelemetry-sdk/tests/metrics/test_meter_provider_fork.pypytest opentelemetry-sdk/tests/trace/test_tracer_provider_fork.pyDoes This PR Require a Contrib Repo Change?
Checklist: