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

feat(attributes) bump semconv to 1.26.0 #6172

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

prestonvasquez
Copy link
Contributor

@prestonvasquez prestonvasquez commented Oct 1, 2024

Resolves #6171, #2165

See issue for migration table.

The specifications for maintaining multiple semantic convention versions is documented here: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md. This pattern does not account for > 2 semantic version update within the lifetime of a major version. Suggest using a registry of attributes upto semconv version. This is a very flexible, lightweight pattern that is easy to update and maintain. If this comes up again, suggest abstracting it.

To use a non-default version:

export OTEL_SEMCONV_STABILITY_OPT_IN=mongo/v1.26.0

To include multiple versions:

export OTEL_SEMCONV_STABILITY_OPT_IN=mongo/v1.26.0,mongo/v1.24.0

To include all versions:

export OTEL_SEMCONV_STABILITY_OPT_IN=mongo/dup

To include the default (v1.17.0) version:

unset OTEL_SEMCONV_STABILITY_OPT_IN

AFAICT the only use-case of this pattern within open-telemetry/opentelemetry-go-contrib is otelhttp/internal/semconv. The pattern used is rather heavy-handed for otelmongo, requiring entirely separate structures to manage attribute propagation.

Benchmarking

The original solution is more performant, with about half the runtime.

// New
ini = appendOpNameAttrs(ini, reg, "opName")
ini = appendDBNamespace(ini, reg, "dbNamespace")
ini = appendNetworkPort(ini, reg, 1)
ini = appendNetworkHost(ini, reg, "host")
ini = appendNetworkTransport(ini, reg)

// Old
ini = append(ini, semconv.DBOperation("opName"))
ini = append(ini, semconv.DBName("dbNamespace"))
ini = append(ini, semconv.NetPeerName("host"))
ini = append(ini, semconv.NetPeerPort(1))
ini = append(ini, semconv.NetTransportTCP)

Benchmark for new:

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo
cpu: Apple M1 Pro
Benchmark_appendAttrs-10          968446              1155 ns/op            3228 B/op          0 allocs/op
PASS
ok      go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo 2.415s

Benchmark for old:

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo
cpu: Apple M1 Pro
Benchmark_appendAttrs-10         1761882               688.6 ns/op          2218 B/op          0 allocs/op

@prestonvasquez prestonvasquez requested a review from a team as a code owner October 1, 2024 20:40
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.6%. Comparing base (64a1336) to head (6ebd6d2).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6172     +/-   ##
=======================================
+ Coverage   67.4%   67.6%   +0.1%     
=======================================
  Files        206     207      +1     
  Lines      13220   13279     +59     
=======================================
+ Hits        8922    8983     +61     
+ Misses      3996    3995      -1     
+ Partials     302     301      -1     
Files with missing lines Coverage Δ
....mongodb.org/mongo-driver/mongo/otelmongo/mongo.go 86.8% <100.0%> (+0.1%) ⬆️
...ongodb.org/mongo-driver/mongo/otelmongo/semconv.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@prestonvasquez prestonvasquez changed the title otelmongo#6171 bump semconv to 1.26.0 chore(deps) bump semconv to 1.26.0 Oct 1, 2024
@prestonvasquez
Copy link
Contributor Author

@pellared @dmathieu following up on issue #5551

@dmathieu
Copy link
Member

dmathieu commented Oct 2, 2024

Updating semconv is definitely not just a chore, especially since, as you mention, there are field name changes (which can cause issues with users, since they need to change their queries and dashboards).

We should either mark these breaking changes explicitely in a changelog entry, or follow a similar migration path as the otelhttp instrumentation is doing: #5132

CHANGELOG.md Outdated Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Oct 2, 2024

We should either mark these breaking changes explicitely in a changelog entry, or follow a similar migration path as the otelhttp instrumentation is doing: #5132

@open-telemetry/specs-semconv-maintainers, @open-telemetry/semconv-db-approvers, do you have some recommendations or feedback regarding changes of database semantic conventions? Are we supposed to do some migrations in instrumentation libraries like for HTTP instrumentations?

CC @XSAM as you are the maintainer of https://github.com/XSAM/otelsql which AFAIK is the most popular instrumentation library for database/sql.

@prestonvasquez prestonvasquez changed the title chore(deps) bump semconv to 1.26.0 feat(attributes) bump semconv to 1.26.0 Oct 2, 2024
@trask
Copy link
Member

trask commented Oct 2, 2024

check out https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md:

Warning

Existing database instrumentations that are using v1.24.0 of this document (or prior):

  • SHOULD NOT change the version of the database conventions that they emit by default until the database semantic conventions are marked stable. Conventions include, but are not limited to, attributes, metric and span names, and unit of measure.
  • SHOULD introduce an environment variable OTEL_SEMCONV_STABILITY_OPT_IN in the existing major version which is a comma-separated list of values. If the list of values includes:
    • database - emit the new, stable database conventions, and stop emitting the old experimental database conventions that the instrumentation emitted previously.
    • database/dup - emit both the old and the stable database conventions, allowing for a seamless transition.
    • The default behavior (in the absence of one of these values) is to continue emitting whatever version of the old experimental database conventions the instrumentation was emitting previously.
    • Note: database/dup has higher precedence than database in case both values are present
  • SHOULD maintain (security patching at a minimum) the existing major version for at least six months after it starts emitting both sets of conventions.
  • SHOULD drop the environment variable in the next major version.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I would rather first have a PR which bumps semconv to v1.24.0

@prestonvasquez
Copy link
Contributor Author

I would rather first have a PR which bumps semconv to v1.24.0

@pellared Could you clarify this?

@trask
Copy link
Member

trask commented Oct 4, 2024

I would rather first have a PR which bumps semconv to v1.24.0

would this go against the semconv recommendation?

SHOULD NOT change the version of the database conventions that they emit by default until the database semantic conventions are marked stable.

@pellared
Copy link
Member

pellared commented Oct 5, 2024

I would rather first have a PR which bumps semconv to v1.24.0

would this go against the semconv recommendation?

SHOULD NOT change the version of the database conventions that they emit by default until the database semantic conventions are marked stable.

I thought that breaking changes started since v1.25.0. But I need to double check it later

@trask
Copy link
Member

trask commented Oct 7, 2024

I thought that breaking changes started since v1.25.0. But I need to double check it later

ah, if that's the case then no problem, since that guidance was just to recommend limiting to a single (large) breaking change, instead of multiple smaller breaking changes

@prestonvasquez
Copy link
Contributor Author

@pellared Do you have an action item for this PR?

@pellared
Copy link
Member

pellared commented Oct 11, 2024

@pellared Do you have an action item for this PR?

Let's first just bump semconv to v1.24.0.

In next PRs we can add support for v1.26.0 (or even newer) and OTEL_SEMCONV_STABILITY_OPT_IN.

Notice also that the OTel Semantic Conventions only defines following values for OTEL_SEMCONV_STABILITY_OPT_IN:

  • database
  • database/dup
  • (none)

@prestonvasquez
Copy link
Contributor Author

prestonvasquez commented Oct 14, 2024

@pellared

Notice also that the OTel Semantic Conventions only defines following values for OTEL_SEMCONV_STABILITY_OPT_IN:

  • database
  • database/dup
  • (none)

IIUC this pattern is misguided and doesn't know how to deal with multiple experimental bumps within a major version. Nor does it explain why that would never happen. See the PR description for more info on the proposed design.

@trask
Copy link
Member

trask commented Oct 14, 2024

@prestonvasquez OTEL_SEMCONV_STABILITY_OPT_IN is a comma-separated list of values, see

https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md

  • SHOULD introduce an environment variable OTEL_SEMCONV_STABILITY_OPT_IN
    in the existing major version which is a comma-separated list of values.
    If the list of values includes:
    • database - emit the new, stable database conventions,
      and stop emitting the old experimental database conventions
      that the instrumentation emitted previously.
    • database/dup - emit both the old and the stable database conventions,
      allowing for a seamless transition.
    • The default behavior (in the absence of one of these values) is to continue
      emitting whatever version of the old experimental database conventions
      the instrumentation was emitting previously.
    • Note: database/dup has higher precedence than database in case both values are present

@prestonvasquez
Copy link
Contributor Author

@prestonvasquez OTEL_SEMCONV_STABILITY_OPT_IN is a comma-separated list of values, see

@trask Would you mind reading the PR description and letting me know if the proposal is in line with the spirit of this requirement?

@pellared
Copy link
Member

This pattern does not account for > 2 semantic version update within the lifetime of a major version.

You would need to address it in https://github.com/open-telemetry/semantic-conventions.

This does not prevent bumping semconv to v1.24.0 beforehand.

@trask
Copy link
Member

trask commented Oct 14, 2024

@trask Would you mind reading the PR description and letting me know if the proposal is in line with the spirit of this requirement?

my recommendation would be to stick to what's defined in the semantic conventions

if you're interested, there was a lot of discussion around the OTEL_SEMCONV_STABILITY_OPT_IN mechanism when it was introduced in open-telemetry/opentelemetry-specification#3404 and open-telemetry/opentelemetry-specification#3443 (and refined in some later PRs)

This pattern does not account for > 2 semantic version update within the lifetime of a major version.

You would need to address it in https://github.com/open-telemetry/semantic-conventions.

I'm happy to discuss if you open an issue in https://github.com/open-telemetry/semantic-conventions describing the limitations you are running into with the existing OTEL_SEMCONV_STABILITY_OPT_IN mechanism.

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.

[otelmongo] bump semconv to 1.26.0
4 participants