-
Couldn't load subscription status.
- Fork 1k
Emit schema url from HTTP instrumentations #15144
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
| .addAttributesExtractors(additionalExtractors) | ||
| .addOperationMetrics(HttpClientMetrics.get()); | ||
| .addOperationMetrics(HttpClientMetrics.get()) | ||
| .setSchemaUrl(SchemaUrls.V1_37_0); |
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.
I think it makes sense to set schema url to the most recent version of schema that the instrumentation conforms to(?)
cc @lmolkova
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.
I've added this to next week's Semantic Convention SIG meeting to get feedback
The test framework was not copying the schema URL from the protobuf ScopeSpans to the InstrumentationScopeInfo when converting spans, causing all schema URL assertions to fail even though spans were correctly emitting schema URLs.
The instrumenterBuilder() method in DefaultHttpClientInstrumenterBuilder was creating new Instrumenter builders without setting the schema URL. This affected connection-level instrumentations (e.g., Netty connection spans for error cases) which use this method to create their instrumenters. Fixes failing tests in finagle-http-23.11 where connection error spans were missing the schema URL.
The TelemetryConverter now correctly preserves schema URLs from protobuf data. The MeterTest files create meters with schema URL 'http://schema.org', so the test expectations need to be updated to match.
Related to #12846