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

Adopt latest semantic conventions #174

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Adopt latest semantic conventions #174

merged 2 commits into from
Jun 25, 2024

Conversation

tt
Copy link
Contributor

@tt tt commented Jun 16, 2024

This adopts the latest semantic conventions, version 1.26.0, and keeps what's already there.

Notably, this pull request makes the following HTTP client span attributes available:

  • http.request.method
  • http.response.status_code
  • server.address
  • server.port
  • url.full
  • url.scheme

There are no constants available for these yet. open-telemetry/opentelemetry-erlang#733 is currently a draft.

See also this summary of changes and the list of deprecated attributes.

I'll note that the migration guide suggests defining the OTEL_SEMCONV_STABILITY_OPT_IN environment variable to allow opting in to the new set. That seems unnecessarily complicated to me but happy to follow that advise if you prefer.

I think the simpler approach is this:

  • The current release, version 1.3.1, contains the old set of attributes ("default behavior")
  • The next release, version 1.3.2, contains both the old and the new set of attributes (http/dup equivalent)
  • The next major release contains only the new set of attributes (http equivalent)

The benefit of making these available already is that anyone adopting Telepoison today can use the latest conventions already and for existing users, double-writing allows for a seamless migration.

@tt tt requested a review from a team as a code owner June 16, 2024 14:33
@cottinisimone cottinisimone requested a review from mbusi June 17, 2024 07:25
Copy link
Contributor

@mbusi mbusi left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 🙏
I think your proposal makes sense 👍
@primait/shared-services I think we should double check that datadog is 100% compatible with the "new" (one year old 😆 ) naming

"server.port",
"url.full",
"url.scheme"
] ==
attributes |> Map.keys() |> Enum.sort()

assert {"http.method", "GET"} in attributes
Copy link
Contributor

Choose a reason for hiding this comment

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

we could maybe start adding comments to attributes that should be deprecated, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

WDYM? How?

Copy link
Contributor

@mbusi mbusi Jun 25, 2024

Choose a reason for hiding this comment

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

I was thinking about simple plain comments like

# deprecated

😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So # deprecated after each old convention in this list?

@http_method Atom.to_string(Conventions.http_method())
@http_request_method "http.request.method"
@http_response_status_code "http.response.status_code"
@http_route Atom.to_string(Conventions.http_route())
@http_status_code Atom.to_string(Conventions.http_status_code())
@http_url Atom.to_string(Conventions.http_url())
@net_peer_name Atom.to_string(Conventions.net_peer_name())
@server_address "server.address"
@server_port "server.port"
@url_full "url.full"
@url_scheme "url.scheme"
@url_template "url.template"

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, something like that 🤷

Copy link
Member

@MaeIsBad MaeIsBad left a comment

Choose a reason for hiding this comment

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

@\primait/shared-services I think we should double check that datadog is 100% compatible with the "new" (one year old 😆 ) naming

yeah it is

https://docs.datadoghq.com/opentelemetry/schema_semantics/semantic_mapping/

"server.port",
"url.full",
"url.scheme"
] ==
attributes |> Map.keys() |> Enum.sort()

assert {"http.method", "GET"} in attributes
Copy link
Member

Choose a reason for hiding this comment

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

WDYM? How?

@mbusi
Copy link
Contributor

mbusi commented Jun 25, 2024

@\primait/shared-services I think we should double check that datadog is 100% compatible with the "new" (one year old 😆 ) naming

yeah it is

https://docs.datadoghq.com/opentelemetry/schema_semantics/semantic_mapping/

not sure about that page should be read, but I think it's the opposite -.-"

 This page lists mappings for OpenTelemetry semantic conventions to Datadog’s semantic conventions.

I don't think it does it 'automagically'; my understanding is you should use datadog's conventions and this page simply lists how to map them from opentelemetry conventions; am I wrong? :D

@MaeIsBad
Copy link
Member

Datadog does map otel semantic conventions to their own format but it looks like the new http conventions aren't supported yet

DataDog/opentelemetry-mapping-go#163

I'll note that the migration guide suggests defining the OTEL_SEMCONV_STABILITY_OPT_IN environment variable to allow opting in to the new set. That seems unnecessarily complicated to me but happy to follow that advise if you prefer.

I think considering that datadog and upstream otel libs don't have support for the new spec yet we'd like to have an opt-in flag

@tt
Copy link
Contributor Author

tt commented Jun 25, 2024

I think considering that datadog and upstream otel libs don't have support for the new spec yet we'd like to have an opt-in flag

I haven't used Datadog but would this matter as long as the old attributes are still there? This pull request doesn't remove them. It only adds the new ones to allow a seamless migration.

@MaeIsBad
Copy link
Member

I haven't used Datadog but would this matter as long as the old attributes are still there? This pull request doesn't remove them. It only adds the new ones to allow a seamless migration.

you're absolutely right, sorry, brain fart

@mbusi can we merge?

@MaeIsBad MaeIsBad requested a review from mbusi June 25, 2024 14:45
@MaeIsBad MaeIsBad merged commit 6943e8e into primait:master Jun 25, 2024
2 checks passed
@tt tt deleted the adopt-latest-semantic-conventions branch June 26, 2024 07:23
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.

3 participants