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

Replace usages of SpanAttributes/MetricAttributes with Attributes #4175

Closed
pichlermarc opened this issue Oct 2, 2023 · 11 comments
Closed

Replace usages of SpanAttributes/MetricAttributes with Attributes #4175

pichlermarc opened this issue Oct 2, 2023 · 11 comments
Assignees
Labels
contribfest These small and isolated issues are suitable for Kubecon Contribfest feature-request never-stale triage:accepted This feature has been accepted

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Oct 2, 2023

SpanAttributes is deprecated, but bumping the minimum API version would be considered breaking. For 2.0 we could replace SpanAttributes with Attributes and bump the minimum API version accordingly.

We can do the same with MetricAttributes


THIS IS A BREAKING CHANGE. PLEASE MAKE PR TO THE next BRANCH FOR INCLUSION IN THE 2.0 RELEASE

Below is a list of files with instances of SpanAttributes or MetricAttributes. In each package, the minimum API version should be bumped to 1.1.0 (first version with the common definition of Attributes) and any instances of SpanAttributes or MetricAttributes should be replaced with Attributes.

Please limit PRs to a single package in order to make them quickly and easily reviewable.
If you are working on one of the below packages, please comment so others know the issue is being worked on.

experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts
experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts
experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
experimental/packages/opentelemetry-instrumentation-grpc/src/grpc-js/index.ts
experimental/packages/opentelemetry-instrumentation-grpc/src/types.ts
experimental/packages/opentelemetry-instrumentation-grpc/test/grpc-protobuf-ts.test.ts
experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts
experimental/packages/opentelemetry-instrumentation-http/src/http.ts
experimental/packages/opentelemetry-instrumentation-http/src/types.ts
experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts
experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts
experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts
packages/opentelemetry-core/src/common/attributes.ts
packages/opentelemetry-core/src/trace/sampler/ParentBasedSampler.ts
packages/opentelemetry-resources/src/Resource.ts
packages/opentelemetry-resources/src/types.ts
packages/opentelemetry-sdk-trace-base/src/export/ReadableSpan.ts
packages/opentelemetry-sdk-trace-base/src/Sampler.ts
packages/opentelemetry-sdk-trace-base/src/sampler/ParentBasedSampler.ts
packages/opentelemetry-sdk-trace-base/src/Span.ts
packages/opentelemetry-sdk-trace-base/src/TimedEvent.ts
packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts
packages/opentelemetry-sdk-trace-base/test/common/Tracer.test.ts
packages/opentelemetry-semantic-conventions/src/resource/SemanticResourceAttributes.ts
packages/opentelemetry-shim-opentracing/src/shim.ts
packages/sdk-metrics/src/aggregator/types.ts
packages/sdk-metrics/src/exemplar/AlignedHistogramBucketExemplarReservoir.ts
packages/sdk-metrics/src/exemplar/AlwaysSampleExemplarFilter.ts
packages/sdk-metrics/src/exemplar/Exemplar.ts
packages/sdk-metrics/src/exemplar/ExemplarFilter.ts
packages/sdk-metrics/src/exemplar/ExemplarReservoir.ts
packages/sdk-metrics/src/exemplar/NeverSampleExemplarFilter.ts
packages/sdk-metrics/src/exemplar/SimpleFixedSizeExemplarReservoir.ts
packages/sdk-metrics/src/exemplar/WithTraceExemplarFilter.ts
packages/sdk-metrics/src/export/MetricData.ts
packages/sdk-metrics/src/Instruments.ts
packages/sdk-metrics/src/ObservableResult.ts
packages/sdk-metrics/src/state/DeltaMetricProcessor.ts
packages/sdk-metrics/src/state/HashMap.ts
packages/sdk-metrics/src/state/MultiWritableMetricStorage.ts
packages/sdk-metrics/src/state/SyncMetricStorage.ts
packages/sdk-metrics/src/state/WritableMetricStorage.ts
packages/sdk-metrics/src/utils.ts
packages/sdk-metrics/src/view/AttributesProcessor.ts
packages/sdk-metrics/test/MeterProvider.test.ts
packages/sdk-metrics/test/state/HashMap.test.ts
packages/sdk-metrics/test/state/MultiWritableMetricStorage.test.ts
packages/sdk-metrics/test/util.ts
packages/sdk-metrics/test/utils.test.ts
@pichlermarc pichlermarc added this to the OpenTelemetry SDK 2.0 milestone Oct 2, 2023
@pichlermarc pichlermarc changed the title Replace usages of SpanAttributes with Attributes Replace usages of SpanAttributes/MetricAttributes with Attributes Oct 3, 2023
@dyladan dyladan added triage:accepted This feature has been accepted and removed triage labels Oct 18, 2023
@dyladan dyladan added the contribfest These small and isolated issues are suitable for Kubecon Contribfest label Nov 8, 2023
@lilacyl
Copy link

lilacyl commented Nov 8, 2023

I'll work on this. Working on experimental/packages/opentelemetry-exporter-prometheus/

@JamieDanielson
Copy link
Member

JamieDanielson commented Jan 9, 2024

maybe we make this into a checklist so it's easier to track?

opentelemetry-exporter-prometheus (#4993)

  • experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts
  • experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts

opentelemetry-instrumentation-fetch (no usages found)

  • experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

opentelemetry-instrumentation-grpc (no usages found)

  • experimental/packages/opentelemetry-instrumentation-grpc/src/grpc-js/index.ts
  • experimental/packages/opentelemetry-instrumentation-grpc/src/types.ts
  • experimental/packages/opentelemetry-instrumentation-grpc/test/grpc-protobuf-ts.test.ts
  • experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts

opentelemetry-instrumentation-http (#5023)

  • experimental/packages/opentelemetry-instrumentation-http/src/http.ts
  • experimental/packages/opentelemetry-instrumentation-http/src/types.ts
  • experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
  • experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts
  • experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts

opentelemetry-instrumentation-xml-http-request

  • experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts

opentelemetry-core (#4408)

  • packages/opentelemetry-core/src/common/attributes.ts
  • packages/opentelemetry-core/src/trace/sampler/ParentBasedSampler.ts

opentelemetry-resources (#4428)

  • packages/opentelemetry-resources/src/Resource.ts
  • packages/opentelemetry-resources/src/types.ts

sdk-trace-base (#5009)

  • packages/opentelemetry-sdk-trace-base/src/export/ReadableSpan.ts
  • packages/opentelemetry-sdk-trace-base/src/Sampler.ts
  • packages/opentelemetry-sdk-trace-base/src/sampler/ParentBasedSampler.ts
  • packages/opentelemetry-sdk-trace-base/src/Span.ts
  • packages/opentelemetry-sdk-trace-base/src/TimedEvent.ts
  • packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts
  • packages/opentelemetry-sdk-trace-base/test/common/Tracer.test.ts

opentelemetry-semantic-conventions (#4175 (comment))

  • packages/opentelemetry-semantic-conventions/src/resource/SemanticResourceAttributes.ts

opentelemetry-shim-opentracing (#4430)

  • packages/opentelemetry-shim-opentracing/src/shim.ts

sdk-metrics (#5021)

  • packages/sdk-metrics/src/aggregator/types.ts
  • packages/sdk-metrics/src/exemplar/AlignedHistogramBucketExemplarReservoir.ts
  • packages/sdk-metrics/src/exemplar/AlwaysSampleExemplarFilter.ts
  • packages/sdk-metrics/src/exemplar/Exemplar.ts
  • packages/sdk-metrics/src/exemplar/ExemplarFilter.ts
  • packages/sdk-metrics/src/exemplar/ExemplarReservoir.ts
  • packages/sdk-metrics/src/exemplar/NeverSampleExemplarFilter.ts
  • packages/sdk-metrics/src/exemplar/SimpleFixedSizeExemplarReservoir.ts
  • packages/sdk-metrics/src/exemplar/WithTraceExemplarFilter.ts
  • packages/sdk-metrics/src/export/MetricData.ts
  • packages/sdk-metrics/src/Instruments.ts
  • packages/sdk-metrics/src/ObservableResult.ts
  • packages/sdk-metrics/src/state/DeltaMetricProcessor.ts
  • packages/sdk-metrics/src/state/HashMap.ts
  • packages/sdk-metrics/src/state/MultiWritableMetricStorage.ts
  • packages/sdk-metrics/src/state/SyncMetricStorage.ts
  • packages/sdk-metrics/src/state/WritableMetricStorage.ts
  • packages/sdk-metrics/src/utils.ts
  • packages/sdk-metrics/src/view/AttributesProcessor.ts
  • packages/sdk-metrics/test/MeterProvider.test.ts
  • packages/sdk-metrics/test/state/HashMap.test.ts
  • packages/sdk-metrics/test/state/MultiWritableMetricStorage.test.ts
  • packages/sdk-metrics/test/util.ts
  • packages/sdk-metrics/test/utils.test.ts

@JamieDanielson
Copy link
Member

packages/opentelemetry-semantic-conventions/src/resource/SemanticResourceAttributes.ts this is an auto-generated file and the references to SpanAttributes are auto-generated from a script. No changes should be made here.

@legendecas
Copy link
Member

I believe ResourceAttributes can be replaced with Attributes as well.

Refs: #4428 (comment)

Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@JamieDanielson
Copy link
Member

JamieDanielson commented Sep 16, 2024

Adding Marc's comment here because it was a really useful explanation:

We're just allowed to bump the API version for all packages except for ./packages, as these are all stable and that's the only place where it's actually breaking and requires us to bump to 2.0. So what you could actually do is bump all versions in

  • ./selenium-tests/ (not public anyway -> API bump is okay)
  • ./experimental/ (not stable - therefore we can break in minor -> API bump is okay)
  • ./integration-tests (not public anyway -> API bump is okay)
  • ./examples/ (they're all examples, not published to NPM -> API bump is okay)

What we cannot do on main is bump the peer-dependency of any package in ./packages (with one exception, see below) because it's breaking AND the packages are 1.x (semver stable) already. So a user with an older version of @opentelemetry/api installed will get an npm error and won't be able to typescript compile anymore once we exchange SpanAttributes with Attributes as the type does not exist on their version of the API.

However, there are a few packages where we can actually exchange the SpanAttributes and MetricAttributes for Attributes on main even if the package is 1.x (semver stable) already, and that's in all packages where we use @opentelemetry/api@>=1.3.0 already, because:

  • we don't need to bump to a later API version (not bumping means we're not breaking)
  • Attributes already exists in 1.3.0, and MetricAttributes is a type-alias for Attributes so it's actually a drop-in replacement that won't cause any compile issues.

Everything else that does not fit these constraints must be done in the next branch as they'll actually be breaking for a stable package. 😅

So, to summarize, we can bump and replace SpanAttributes and MetricAttributes on main everywhere except for:

  • @opentelemetry/context-async-hooks is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/context-zone is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/context-zone-peer-dep is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/core is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/exporter-jaeger is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/exporter-zipkin is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/propagator-b3 is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/propagator-jaeger is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/resources is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/sdk-trace-base is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/sdk-trace-node is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/sdk-trace-web is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/propagator-aws-xray is 1.x and depends on API >=1.0.0 (no Attributes type)
  • @opentelemetry/shim-opentracing is 1.x and depends on API >=1.0.0 (no Attributes type)

These changes would have to go into the next branch and will be released with 2.0.

There is a special case with @opentelemetry/sdk-metrics which is 1.x but it's already depending on API >=1.3.0, so the Attributes type is available. Since that type is available (and there's not difference between Attributes and MetricAttributes) we can just replace all usages of MetricAttributes to Attributes in @opentelemetry/sdk-metrics without any repercussions. 🙂

@david-luna
Copy link
Contributor

david-luna commented Sep 27, 2024

opentelemetry-instrumentation-xml-http-request

  • experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts

In main branch

cd experimental/packages/opentelemetry-instrumentation-xml-http-request/
rg SpanAttributes
src/xhr.ts
162:  _addFinalSpanAttributes(span: api.Span, xhrMem: XhrMem, spanUrl?: string) {
418:        plugin._addFinalSpanAttributes(span, xhrMem, spanUrl);

rg MetricAttributes
# no results
rg ResourceAttributes
# no results
rg SpanAttributeValue
# no results
rg MetricAttributeValue
# no results

I think instrumentation-xml-http-request is fine :)

@david-luna
Copy link
Contributor

david-luna commented Sep 27, 2024

opentelemetry-instrumentation-grpc

  • experimental/packages/opentelemetry-instrumentation-grpc/src/grpc-js/index.ts
  • experimental/packages/opentelemetry-instrumentation-grpc/src/types.ts
  • experimental/packages/opentelemetry-instrumentation-grpc/test/grpc-protobuf-ts.test.ts
  • experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts

instrumentation-grpc seems to be clear as well

cd experimental/packages/opentelemetry-instrumentation-grpc/
rg SpanAttributes
src/types.ts
27:  metadataToSpanAttributes?: {

src/instrumentation.ts
537:          config.metadataToSpanAttributes?.client?.requestMetadata ?? []
541:          config.metadataToSpanAttributes?.client?.responseMetadata ?? []
547:          config.metadataToSpanAttributes?.server?.requestMetadata ?? []
551:          config.metadataToSpanAttributes?.server?.responseMetadata ?? []

README.md
54:| [`metadataToSpanAttributes`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-grpc/src/types.ts#L27) | `object`          | List of case insensitive metadata to convert to span attributes. Client and server (outgoing requests, incoming responses) metadata attributes will be converted to span attributes in the form of `rpc.{request\response}.metadata.metadata_key`, e.g. `rpc.response.metadata.date` |

test/helper.ts
961:          metadataToSpanAttributes: {

test/grpc-protobuf-ts.test.ts
789:        metadataToSpanAttributes: {

rg MetricAttributes
# no results
rg ResourceAttributes
# no results
rg SpanAttributeValue
# no results
rg MetricAttributeValue
# no results

No types to rename

@david-luna
Copy link
Contributor

david-luna commented Sep 27, 2024

opentelemetry-instrumentation-fetch

  • experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

instrumentation-fetch too

cd experimental/packages/opentelemetry-instrumentation-fetch/
rg SpanAttributes
src/fetch.ts
124:  private _addFinalSpanAttributes(
290:    this._addFinalSpanAttributes(span, response);

rg MetricAttributes
# no results
rg ResourceAttributes
# no results
rg SpanAttributeValue
# no results
rg MetricAttributeValue
# no results

@david-luna
Copy link
Contributor

@JamieDanielson could I be missing something in my previous comments?

with #4978 merged I can go for the http instrumentation :)

@pichlermarc
Copy link
Member Author

@david-luna yep, I everything seems to be done - thanks for taking care of so many of these. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribfest These small and isolated issues are suitable for Kubecon Contribfest feature-request never-stale triage:accepted This feature has been accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants