beyla icon indicating copy to clipboard operation
beyla copied to clipboard

Use semconv in integration tests

Open carrbs opened this issue 11 months ago • 5 comments

Fixes: #630

This is a suggestion for possibly changing the scope of this ticket to start using the opentelemetry-go semconv definitions generated from semantic conventions defined by OpenTelemetry.

The PR introduces a few helper methods that convert attribute.KeyValues into jaeger.Tags. It further exercises these functions several times in one of the integration test files to give an example of usage.

If this seems like an appropriate direction to take, I can finalize the PR (likely just applying the approach to the rest of this test file), and assist with rescoping the issue and opening tickets to utilize this pattern in other files.

A few thoughts if we move forward on this:

  1. I wasn't sure how to handle a one of the tags (jaeger.Tag{Key: "span.kind", Type: "string", Value: "server"}) because it wasn't defined in the semantic-conventions.
  2. If there's a more appropriate location for the helper methods let me know and I can move them.

carrbs avatar Mar 02 '24 01:03 carrbs

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 02 '24 01:03 CLAassistant

Hi @mariomac! 👋

I was also thinking about how we could best handle this. One thought I had was adding the stability as an attribute within the generated semconv. This could be utilized in a step during testing: if the stability is deprecated, tests could fail (or warn) triggering a task to update the name. Would this be a useful approach?


Regarding this:

One advantage of keeping the tag names hardcoded in the tests is that we could detect any breaking change derived from the update of the OTEL libraries and properly fix or document it.

Can you help me understand how the hardcoded names help to detect the breaking changes? When OTEL libraries are updated, the generated semconv is also updated. Deprecation in the generated files is scoped to the comments, for example:

// github.com/open-telemetry/opentelemetry-go/tree/main/semconv/v1.24.0/resource.go
// ...
// Span attributes used by non-OTLP exporters to represent OpenTelemetry Scope's concepts.
const (
	// Deprecated, use the `otel.scope.name` attribute.
	//
	// Type: string
	// RequirementLevel: Optional
	// Stability: deprecated
	// Examples: 'io.opentelemetry.contrib.mongodb'
	OtelLibraryNameKey = attribute.Key("otel.library.name")
	// Deprecated, use the `otel.scope.version` attribute.
	//
	// Type: string
	// RequirementLevel: Optional
	// Stability: deprecated
	// Examples: '1.0.0'
	OtelLibraryVersionKey = attribute.Key("otel.library.version")
)

carrbs avatar Mar 04 '24 16:03 carrbs

Hi @carrbs! The issue here es that even if we argue that we are compliant with the latest stable specifications of OTEL, any breaking change produced after upgrading (and they are frequent), could break the stored queries in the dashboards and alerts of our users.

Imagine we are using version X of OTEL libraries, and we use the unstable semconv.Example constant whose value is example. The users manually set this field name in their queries.

Then we update to the version Y of the libraries, which modified the semconv.Example value to request.example. The next Beyla release would break users' queries.

If we used the semconv.Example constant in our integration tests, this change could have been unnoticed to us, as both producer and tests values are updated automatically and the tests passes. The next Beyla release would introduce an unnoticed breaking change.

If we directly used the example value in the tests, an update in the library would break our integration tests and we could decide what to do: hold the change for a later release, or to cut a major release and document that breaking change so our users are prepared.

It's true that the usual way to proceed would be to just deprecate the Example constant with its original value and create a new semconv.RequestExample constant. But even if that's the usual process, I'd prefer to keep with the hardcoded names as a safety net.

mariomac avatar Mar 05 '24 08:03 mariomac

Hi @mariomac! Thank you for sharing this context with me, I totally understand your concern for the users.

I believe there's more flexibility here. We wouldn't need to be compliant with the latest stable specifications of OTEL, in fact we can specifically say which version of the semantic conventions are supported by beyla, i.e.:

import semconv "go.opentelemetry.io/otel/semconv/v1.24.0"

The semantic conventions in this directory are static and when new versions of the conventions are published, new definitions are generated (and added to a new directory) in the OTEL Go library.

So as an example, if semconv.Example == "example" in version 42, and we say "beyla supports the OTEL semantic conventions version 42", even if version 43 changes semconv.Example == "request.example", we don't need to change to version 43 until we're ready (e.g., a major release with other breaking changes).

Another thought: the way it is currently, (with hardcoded strings), doesn't let the users know which version of the semantic conventions are supported, which could also lead to confusion.

Let me know your thoughts! If you'd like to just have this PR be a small change to the string ( i.e. :s/otel.library.name/otel.scope.name), I can update it that way as well.

carrbs avatar Mar 05 '24 18:03 carrbs

Hi @carrbs ! Yeah, fixing the referred issue should be to just do that change, you can do it and we can merge it when we are ready to publish/report/document the breaking change.

Thank you very much for you collaboration!

mariomac avatar Mar 06 '24 09:03 mariomac

Hey @carrbs, any plans to keep working on this? cheers

marctc avatar Jan 07 '25 10:01 marctc

Closing due inactivity, please reopen if you want to continue working on it. Thanks a lot for contributing :pray:

marctc avatar Jan 14 '25 09:01 marctc