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

HTTP SemConv transition plan is not extensible #3488

Closed
dyladan opened this issue May 9, 2023 · 8 comments · Fixed by open-telemetry/semantic-conventions#104
Closed

HTTP SemConv transition plan is not extensible #3488

dyladan opened this issue May 9, 2023 · 8 comments · Fixed by open-telemetry/semantic-conventions#104
Assignees
Labels
area:configuration Related to configuring the SDK semconv:HTTP spec:miscellaneous For issues that don't match any other spec label

Comments

@dyladan
Copy link
Member

dyladan commented May 9, 2023

The HTTP SemConv transition plan defines an environment variable with potential values http, http/dup, etc. It is my understanding that the plans for future semconv areas like messaging will add additional possible values to this environment variable ( e.g. OTEL_SEMCONV_STABILITY_OPT_IN =http/dup/messaging) source.

After the transition plan merged, it was pointed out to me by @Flarna that the variable requires a predefined method for multiple values such as comma-separated values. Otherwise an implementation which simply checks env.OTEL_SEMCONV_STABILITY_OPT_IN == "http" or uses a switch/case will be broken in the case that multiple values exist as in the example above.

Action items:

  • Document in specification/configuration/sdk-environment-variables.md
  • Add explicit comma separated format
    • Alternatively: explicitly decide we are not going to extend it in the future

/cc @trask

@dyladan dyladan added the spec:miscellaneous For issues that don't match any other spec label label May 9, 2023
@dyladan
Copy link
Member Author

dyladan commented May 9, 2023

It should also be documented in specification/configuration/sdk-environment-variables.md

@arminru arminru added the area:configuration Related to configuring the SDK label May 9, 2023
@mateuszrzeszutek
Copy link
Member

Wasn't that exactly the point behind this env var?
I was under the impression that this was a one-time exercise done only because the HTTP semconv and instrumentations are so widespread, and that we're not going to repeat that approach when stabilizing other conventions (like messaging or rpc).

@dyladan
Copy link
Member Author

dyladan commented May 9, 2023

Wasn't that exactly the point behind this env var?
I was under the impression that this was a one-time exercise done only because the HTTP semconv and instrumentations are so widespread, and that we're not going to repeat that approach when stabilizing other conventions (like messaging or rpc).

If you click through the link in the OP you will see that may not be the case.

@trask
Copy link
Member

trask commented May 9, 2023

I was under the impression that this was a one-time exercise done only because the HTTP semconv and instrumentations are so widespread, and that we're not going to repeat that approach when stabilizing other conventions (like messaging or rpc).

this was an earlier plan, but it changed last week to support future csv values

Otherwise an implementation which simply checks env.OTEL_SEMCONV_STABILITY_OPT_IN == "http" or uses a switch/case will be broken in the case that multiple values exist as in the example above.

this is a great point, I'll send a PR to fix this

@trask
Copy link
Member

trask commented May 9, 2023

Action items:

  • Document in specification/configuration/sdk-environment-variables.md

I'm not sure about this since it's not an SDK environment variable

  • Add explicit comma separated format

I've sent #3492 👍

@mateuszrzeszutek
Copy link
Member

If this is supposed to be extensible, should we remove the

  • SHOULD drop the environment variable in the next major version (stable
    next major version SHOULD NOT be released prior to October 1, 2023).

part then? I am not convinced that we can stabilize (let alone implement) messaging semconv before October.

Also, what's going to happen when we do release a 2.0 instrumentation package? If the env var stays, should the http http/dup options be removed?

@dyladan
Copy link
Member Author

dyladan commented May 10, 2023

If this is supposed to be extensible, should we remove the

  • SHOULD drop the environment variable in the next major version (stable
    next major version SHOULD NOT be released prior to October 1, 2023).

part then? I am not convinced that we can stabilize (let alone implement) messaging semconv before October.

Not just messaging. This will likely be an ongoing concern for a long time as we stabilize all the semconv

Also, what's going to happen when we do release a 2.0 instrumentation package? If the env var stays, should the http http/dup options be removed?

I would think so

@mateuszrzeszutek
Copy link
Member

(I know that #3443 has my approval on it, but I approved the previous version of that warning)

@trask I think that warning (and env var) as it is now is rather confusing for instrumentation maintainers, as it mixes several the HTTP-specific concerns and the general usage of the env var.

  • SHOULD maintain (security patching at a minimum) the existing major version
    for at least six months after it starts emitting both sets of attributes.
  • SHOULD drop the environment variable in the next major version (stable
    next major version SHOULD NOT be released prior to October 1, 2023).

I think these two points should be rephrased so that they only apply to the http http/dup values, not the env var itself. Perhaps the v1.20.0 warning should also apply to chosen values, not the env var itself (as new to-be-stable semconvs will set different support dates/releases)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment