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

Use OTel's semantic convention constants when we bump minimum OTel version #13287

Open
7 tasks
alevenberg opened this issue Dec 13, 2023 · 3 comments
Open
7 tasks
Labels
next major: breaking change this is a change that we should wait to bundle into the next major version type: cleanup An internal cleanup or hygiene concern.

Comments

@alevenberg
Copy link
Member

alevenberg commented Dec 13, 2023

In the async batch publish, we create a root span using the string instead of the constant, since it does not exist in the oldest deps.

https://github.com/googleapis/google-cloud-cpp/pull/13285/files#diff-3e8aefbb8885925f3b6c48b310f1fe9e031aa24315c084fa04cfc13ea5bd86f6R63-R66

Current:

opentelemetry::context::Context root_context;
  // TODO(https://github.com/googleapis/google-cloud-cpp/issues/13287): Use the constant instead of the string.
  root_context.SetValue(/*opentelemetry::trace::kIsRootSpanKey=*/"is_root_span", true);
  options.parent = root_context;

Suggestion:

  root_context =
      root_context.SetValue(opentelemetry::trace::kIsRootSpanKey, true);
  options.parent = root_context;

  • /*sc::kNetworkTransport=*/"network.transport"
  • /*sc::kHttpRequestMethod=*/"http.request.method"
  • /*sc::kUrlFull=*/"url.full"
  • /*sc::kServerAddress=*/"server.address"
  • /*sc::kServerPort=*/"server.port"
  • /*sc::kClientAddress=*/"client.address"
  • /*sc::kClientPort=*/"client.port"
@alevenberg alevenberg added type: cleanup An internal cleanup or hygiene concern. next major: breaking change this is a change that we should wait to bundle into the next major version labels Dec 13, 2023
@dbolduc
Copy link
Member

dbolduc commented Dec 13, 2023

There are a few more of these semantic convention names we would like to update when possible. Can we have this issue encompass all of them? A checklist would be cool.

@alevenberg
Copy link
Member Author

alevenberg commented Dec 13, 2023

We could add comments with issue in the code instead?

@dbolduc
Copy link
Member

dbolduc commented Dec 13, 2023

We could add comments with issue in the code instead?

The code is already ugly, let's not make it fugly. It will be sufficient to have a list of strings to find and replace. I started a checklist for the semantic conventions I reverted in #12992.

@dbolduc dbolduc changed the title [pubsub] Use opentelemetry::trace::kIsRootSpanKey when we make the next otel oldest deps upgrade Use OTel's semantic convention constants when we bump minimum OTel version Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next major: breaking change this is a change that we should wait to bundle into the next major version type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

2 participants