General tag changes #1562
General tag changes #1562
Conversation
To meet opentelemetry standards, 'span.kind' tag needs to be set in all spans. Adds new constant 'SpanKind' with value 'span.kind' in tags.go.
Determines 'span.kind' value for gqlgen is 'server' Sets tag "'span.kind':'server'" for root spans generated from gqlgen Sets tag "'component':'gqlgen'" for all spans generated from gqlgen Modifies tests to check parent and child spans for newly added tags Passes modified test
…gs for google-cloud-go/pubsub Determines 'span.kind' value for google-cloud-go/pubsub is 'producer' or 'consumer' depending on context Sets tag "'span.kind':'producer'" for root span created from Publish in google-cloud-go/pubsub Sets tag "'span.kind':'consumer'" for final span created from WrapRecieveHandler in ggoogle-cloud-go/pubsub Sets tag "'component':'google-cloud-go/pubsub'" for all spans generated from google-cloud-go/pubsub Modifies tests to check parent and child spans for newly added tags Passes modified test
… for go-elasticsearch Determines 'span.kind' value for go-elasticsearch is 'client' Sets tag "'span.kind':'client'" for root spans generated from go-elasticsearch Sets tag "'component':'go-elasticsearch'" for all spans generated from go-elasticsearch Modifies tests to check spans for newly added tags Passes modified test and integration tests for elasticsearch_v6 and elasticsearch_v7
Determines 'span.kind' value for all versions of go-redis is 'client' Sets tag "'span.kind':'client'" for root spans generated from all versions of go-redis Sets tag "'component':'go-redis'" for all spans generated from all versions of go-redis Modifies tests to check spans for newly added tags Passes modified test and integration tests for redis, redis.v7, redis.v8
…' tags for mongo-go-driver Determines 'span.kind' value for mongo-go-driver is 'client' Sets tag "'span.kind':'client'" for root spans generated from mongo-go-driver Sets tag "'component':'mongo-go-driver'" for all spans generated from mongo-go-driver Modifies tests to check spans for newly added tags Passes modified test and integration tests for mongo with v4.4.6 (Version did not require AVX support)
Determines 'span.kind' value for gorilla/mux is ‘server’ Sets tag "'span.kind':'server'" for root spans generated from gorilla/mux Sets tag "'component':'gorilla/mux'" for all spans generated from gorilla/mux Modifies tests to check spans for newly added tags Passes modified test
Determines 'span.kind' value for net/http is ‘server’ or ‘client’ Sets tag "'span.kind':'client'" for root spans generated from roundtripper Sets tag "'span.kind’:’server’” for root spans generated from servehttp or wrapHandler Sets tag "'component’:’net/http’” for all spans generated from net/http Modifies tests to check spans for newly added tags Passes modified test
…afka-go Determines 'span.kind' value for kafka-go is ‘producer’ or ‘consumer’ Sets tag "'span.kind':'producer'" for root spans generated from producer Sets tag "'span.kind’:’consumer’” for root spans generated from consumers Sets tag "'component’:’kafka-go’” for all spans generated from kafka-go Modifies tests to check spans for newly added tags Passes modified test and integrations test
…kind' tags for confluent-kafka-go Determines 'span.kind' value for confluent-kafka-go is ‘producer’ or ‘consumer’ Sets tag "'span.kind':'producer'" for root spans generated from producer Sets tag "'span.kind’:’consumer’” for root spans generated from consumers Sets tag "'component’:’confluent-kafka-go’” for all spans generated from confluent-kafka-go
Determines 'span.kind' value for aws is ‘client’ Sets tag "'span.kind':'client'" for root spans generated from aws Sets tag "'component’:’aws-sdk-go’” for all spans generated from aws-sdk-go Sets tag "'component’:’aws-sdk-go-v2’” for all spans generated from aws-sdk-go-v2 Modifies tests to check spans for newly added tags Passes modified test
…s for gomemcache Determines 'span.kind' value for gomemcache is ‘client’ Sets tag "'span.kind':'client'" for root spans generated from gomemcache Sets tag "'component’:’gomemcache’” for all spans generated from gomemcache Modifies tests to check spans for newly added tags Passes modified test and integration tests
Determines 'span.kind' value for database/sql is ‘client’ Sets tag "'span.kind':'client'" for root spans generated from database/sql Sets tag "'component’:’database/sql’” for all spans generated from database/sql Modifies tests to check spans for newly added tags Passes modified test and integration tests
…restful Determines 'span.kind' value for go-restful is ‘server’ Sets tag "'span.kind’:’server’” for root spans generated from go-restful Sets tag "'component’:’go-restful’” for all spans generated from go-restful Modifies tests to check spans for newly added tags Passes modified test
Determines 'span.kind' value for redigo is ‘client’ Sets tag "'span.kind’:’client’” for root spans generated from redigo Sets tag "'component’:’redigo’” for all spans generated from redigo Modifies tests to check spans for newly added tags Passes modified test
Determines 'span.kind' value for gin is ‘client’ Sets tag "'span.kind’:’client’” for root spans generated from gin Sets tag "'component’:’gin’” for all spans generated from gin Does not set span.kind tag for spans from HTML rendering as that is a child span Modifies tests to check spans for newly added tags Passes modified test
Adds constants to be used for 'span_kind' tags used for identification Values are either server, client, producer, consumer, or internal
Replaces string values in 'span_kind' tags with constants defined in ddtrace/ext/span_kind.go Adds version to component tag for go-redis
…ce-go into general-tag-changes
Renames constants to meet linter requirement
Replaces spankind constants with new values set in previous commit Fixes spankind values for redis to correctly reflect producer/consumer Passes segmention/kafka.go.v0 tests
Changes 'span.kind' string literal to ext.SpanKind in tests for various integrations
Fixes order of imports to meet linting standard
Determines 'span.kind' value for mgo is ‘client’ Sets tag "'span.kind’:’client’” for root spans generated from mgo Sets tag "'component’:’mgo’” for all spans generated from mgo Does not set span.kind for spans created from Iter() because those generate child spans after Modifies tests to check spans for newly added tags Adds new test to check no span.kind tag is set for Iter() Passes modified test and integration tests
Changes pre-existing span.kind server string literal to constant ext.SpanKindServer
There was a problem hiding this comment.
It would be good to standardize the format of the Component a little more.
Currently, some of them are just the last part of the package:
"gqlgen" -> contrib/99designs/gqlgen
Some of them include path components:
"Shopify/sarama" -> contrib/Shopify/sarama
And some of them contain parts that aren't in the package path at all:
"google-cloud-go/pubsub" -> contrib/cloud.google.com/go/pubsub.v1
I'm thinking the full package name after the contrib/ part might be best.
Open to other ideas.
| span.setMetric(ext.Pid, float64(t.pid)) | ||
| span.setMeta("language", "go") | ||
|
|
There was a problem hiding this comment.
We could use some information about why this is changing, as it doesn't seem to be related to the broader opentelemetry-related tag stuff.
- Why are we now setting pid/language tags on all spans?
- Why is the pid now a metric rather than metadata?
- Why is pid changing from
system.pidtoprocess_id?
There was a problem hiding this comment.
process_id needs to be a numeric value, and since span.setMeta only sets string values, we have to use span.setMetric to set the process_id as a numeric value. The name change and the idea of setting the tags on all spans is to standardize the values across all language tracers
There was a problem hiding this comment.
I'm definitely lacking context here, just want to make sure these two questions aren't about breaking changes. Also would this require a change in the trace-agent? Should we keep both fields instead of replacing?
Why is the pid now a metric rather than metadata?
Why is pid changing from system.pid to process_id?
Would you confirm @knusbaum ?
There was a problem hiding this comment.
Another detail about the system.pid, even though we are just renaming it with this PR, the backend has already been remapping and duplicating the value "system.pid" to "process.id" so it wouldn't produce any breaking changes. Renaming the tag and changing its value is fine as other tracers already have numerical process_id tags.
There was a problem hiding this comment.
This doesn't answer the numeric change. Are we sure this needs to be a metric and not metadata? It was previously metadata. Fields like this that are used to connect things sometimes are very particular.
I also still have concerns (put in slack) about this change, as it doesn't seem to match the spec, and I'm hoping we won't need to do this at all.
…e/redigo Determines span.kind tag for gomodule/redigo as ‘client’ Sets tag “’span.kind’:’client’” for all root spans generated from gomodule/redigo Sets tag "'component’:’gomodule/redigo’” for all spans generated from gomodule/redigo Modifies tests to check spans for newly added tags Passes modified test and integration tests
Updates README.md to include specifications for obligatory 'span.kind' and 'component' tags
Updates all 'component' values to be set to full path of integration package starting after contrib/ Runs all tests and passes them
Implemented 'component' change to all libraries as explained above after reaching a group consensus Added changes to contrib/README.md to ensure further contributions implement the mandatory 'span.kind' and 'component' tags |
| span.setMetric(ext.Pid, float64(t.pid)) | ||
| span.setMeta("language", "go") | ||
|
|
There was a problem hiding this comment.
This doesn't answer the numeric change. Are we sure this needs to be a metric and not metadata? It was previously metadata. Fields like this that are used to connect things sometimes are very particular.
I also still have concerns (put in slack) about this change, as it doesn't seem to match the spec, and I'm hoping we won't need to do this at all.
Process_id needs to be a numeric value as per new specifications. The meta tags of a span are stored in a string-to-string map which means that numerical values cannot be put into it. Instead, the metric tags of a span are in a string-to-numeric map which is where other numerical values are stored. Thus we use In terms of the backend, other tracers already sent process_id through the metric tags so there isn't an issue there. In fact, the go tracer is the only one sending process_id as a string via the meta tags which is why to standardize everything across tracers, the value is being changed to a numeric value sent via the metric tags |
There was a problem hiding this comment.
In terms of the backend, other tracers already sent process_id through the metric tags so there isn't an issue there. In fact, the go tracer is the only one sending process_id as a string via the meta tags which is why to standardize everything across tracers, the value is being changed to a numeric value sent via the metric tags
This was what I was looking for. Thanks for clarifying. This LGTM now.
BenchmarksFound 0 performance improvements and 6 performance regressions! Performance is the same for 0 cases. scenario:BenchmarkConcurrentTracing-24
scenario:BenchmarkStartSpan-24
|
What does this PR do?
Adds 'component' and 'span.kind' tags to several integrations to meet opentelemetry standards.
Modifies tests to check for newly added tags per integration
Component is simply the library/integration name the span originated from.
Span.kind is better explained here where it mentions the different types
Changes (string) system.pid to (float64) process_id as per new tracer requirements
Changes system tests to include testing for newly added 'component' and 'span.kind' tags
Motivation
The tracer should meet the opentelemetry standard as more people adapt it and we want all information that users expect to be available for as many instances as possible
Describe how to test/QA your changes
Tests for relevant libraries have been updated and passed. As long as the resulting spans have all values that are expected as described in the aforementioned link, things should be good.
Reviewer's Checklist
Triagemilestone is set.