Skip to content
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

[forward port to master] tracing: allow the Resource to be set externally #4496

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

thaJeztah
Copy link
Member

This is a workaround for the brittleness when constructing OTel Resource objects. Internally, the OTel libraries do their own detection which can be merged with one created in code. However, the semconv spec versions must match. (NOT module version! the semconv package has multiple subpackages for each spec version, e.g. semconv/v1.17, semconv/v1.21, etc.)

This creates a problem when BuildKit is used as a library - the importing app might be using a different, otherwise compatible version of the OTel libraries, so when it creates a resource, it will be merged with one of a different version.

By allowing the Resource to be set (like the Recorder), the calling code can construct a resource using known consistent library versions that work, and then allow BuildKit to take over the rest of the initialization process for OTel.

(cherry picked from commit 7b3fe03)

@thaJeztah
Copy link
Member Author

@laurazard @milas @tonistiigi PTAL

@laurazard
Copy link
Member

@thaJeztah looks like this needs another change around

opts = append(opts, sdkmetric.WithResource(res))

@thaJeztah
Copy link
Member Author

oh! indeed; yeah, master diverged somewhat, but I missed that that part was added. Let me fix

This is a workaround for the brittleness when constructing OTel
`Resource` objects. Internally, the OTel libraries do their own
detection which can be merged with one created in code. However,
the `semconv` spec versions must match. (NOT module version! the
`semconv` package has multiple subpackages for each spec version,
e.g. `semconv/v1.17`, `semconv/v1.21`, etc.)

This creates a problem when BuildKit is used as a library - the
importing app might be using a different, otherwise compatible
version of the OTel libraries, so when it creates a resource, it
will be merged with one of a different version.

By allowing the `Resource` to be set (like the `Recorder`), the
calling code can construct a resource using known consistent
library versions that work, and then allow BuildKit to take over
the rest of the initialization process for OTel.

Signed-off-by: Milas Bowman <[email protected]>
(cherry picked from commit 7b3fe03)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crazy-max crazy-max merged commit 957cb50 into moby:master Dec 19, 2023
60 checks passed
@thaJeztah thaJeztah deleted the forward_port_otel_resource branch December 19, 2023 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants