-
Notifications
You must be signed in to change notification settings - Fork 925
reduce service detector requirement for instance id #4570
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?
Conversation
the service resource detector SHOULD populate service.instance.id, however some runtimes (notably those frequently used for PHP) use a shared-nothing approach. This leads to a new instance id for each request.
Think this may also affect lambda environments where each request potentially is a new service instance. |
Seems to me that rather than unpopulating service instance id, this raises the question of what a service instance is. If it is being triggered on each new request, that's probably not correct. But there is likely still some concept of a service instance. In lambda for example, it might be a new instance if there is a new deployment. |
I think its a good default for SDKs to include a Alternatively, the |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
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.
Should we consider doing the same for the service name that appears just before this?
@jpkrohling service name can already be set by an environment variable, so doesn't need this? I liked @jack-berg 's idea that sounds to me like making instance id similarly be able to be set. Then there is no need to make it optional. |
Or no, can you not already do
The explanation of priority between this and the detectors is not clear as far as I can tell. The section on priority calls the env var "secondary" and seems to be referring to other resource information as what is provided by a user and the env var is somehow not provided by the user...
But maybe I'm just reading this wrong and its meant to say env var attributes take precedence over other detected reosurces and those defined by the SDK like If others agree with this being unclear wording I can send a PR to try clearing it up, if my reading of it is correct. |
This is the bit of the spec that I'm struggling with:
My interpretation is:
We have a bunch of optional and 3rd-party detectors, which are all executed before the service detector. If any of them were to provide
You could, that would give you a stable service instance id. But I don't think we can implement a detector to do the same, assuming the order of execution I've interpreted... 😕 |
Fixes #
Changes
The service resource detector SHOULD populate service.instance.id, however some runtimes (notably those frequently used for PHP) use a shared-nothing approach. This leads to a new instance id for each request, eg open-telemetry/opentelemetry-php#1643
Changing the requirement from an implied MUST to SHOULD allows implementations to deviate when necessary, which I plan to do for PHP SIG.
For non-trivial changes, follow the change proposal process.
CHANGELOG.md
file updated for non-trivial changesspec-compliance-matrix.md
updated if necessary