-
Notifications
You must be signed in to change notification settings - Fork 7
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
Use otel4s-semconv-metrics-experimental
for semantic testing
#129
base: main
Are you sure you want to change the base?
Conversation
@@ -179,7 +179,7 @@ object OtelMetrics { | |||
Meter[F] | |||
.upDownCounter[Long](s"http.$kind.active_requests") | |||
.withUnit("{request}") | |||
.withDescription("Number of active HTTP requests.") | |||
.withDescription(s"Number of active HTTP $kind requests.") |
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.
We've spotted the first mismatch 🥳
The current implementation of the MetricOps is rather limiting and we cannot comply the spec. I don't think we should make drastic changes to 0.23, but we can to |
it seems I'll need to poke at updating the implementations of the tracing middlewares to be correct |
f52eb1d
to
6497001
Compare
6497001
to
904b670
Compare
@iRevive I rebased, fixed the conflicts, and force pushed. how do you want to proceed given that the current implementation from 0.23.x doesn't support some attributes? should we add logic to specifically skip the checks for those attributes and add a TODO comment to fix it in a later release? should we wait until we actually have a compliant implementation to fix it? something else?
it should be possible to do it in a binary compatible way in 0.23.x, though it would be a pain. but would it be worth it anyway, as it may yet be many years until 1.0.x is released? |
I think it would be the easiest option for us. |
Here is an example of the 'semantic conventions tests'.