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

distributedtracing: adding distributed tracing resource attributes #6947

Merged

Conversation

brettmc
Copy link
Contributor

@brettmc brettmc commented Aug 21, 2024

Adding a resource_attributes map to the distributed_tracing config. Entries in this map will be passed through to the OpenTelemetry SDK where they will be added as resource attributes. Resource attributes are typically service.namespace, service.version, service.name and service.instance.id, see https://opentelemetry.io/docs/specs/semconv/resource/

Fixes: #6942

Why the changes in this PR are needed?

Allows extra OpenTelemetry resource attributes to be added to a trace, such as service.namespace

What are the changes in this PR?

Adds an optional config distributed_tracing.resource_attributes, which is map of strings. Values here are
added via the opentelemetry SDK as resource attributes.

Notes to assist PR review:

I didn't find any tests which verify the telemetry (trace) output, but I did apply the following config and observe this output:

config:
distributed_tracing:
  type: grpc
  address: localhost:4317
  service_name: opa
  resource_attributes:
    service.name: foobar
    service.namespace: foo
    service.version: "1"
  sample_percentage: 50
  encryption: "off"
output in opentelemetry collector: NB resource attributes, and that service_name takes precedence over resource_attributes."service.name"
2024-08-21T05:54:36.603Z	info	ResourceSpans #0
Resource SchemaURL: 
Resource attributes:
     -> service.name: Str(opa)
     -> service.namespace: Str(foo)
     -> service.version: Str(1)
ScopeSpans #0
ScopeSpans SchemaURL: 
InstrumentationScope go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp 0.53.0
Span #0
    Trace ID       : 947eef4950fa437411a025bc499e6530
    Parent ID      : 
    ID             : 539d178e352cfac6
    Name           : v1/data
    Kind           : Server
    Start time     : 2024-08-21 05:54:32.602923136 +0000 UTC
    End time       : 2024-08-21 05:54:32.602987413 +0000 UTC
    Status code    : Unset
    Status message : 
Attributes:
     -> http.method: Str(POST)
     -> http.scheme: Str(http)
     -> net.host.name: Str(localhost)
     -> net.host.port: Int(8181)
     -> net.sock.peer.addr: Str(127.0.0.1)
     -> net.sock.peer.port: Int(55730)
     -> user_agent.original: Str(curl/7.81.0)
     -> http.target: Str(/v1/data/my_policy)
     -> net.protocol.version: Str(1.1)
     -> http.request_content_length: Int(28)
     -> http.response_content_length: Int(3)
     -> http.status_code: Int(200)
### Further comments:

Copy link

netlify bot commented Aug 21, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 92b48d3
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/66ccebce2053990008d7556d
😎 Deploy Preview https://deploy-preview-6947--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ashutosh-narkar ashutosh-narkar force-pushed the otel-resource-attributes branch 2 times, most recently from 9baba2d to f8ee0b3 Compare August 21, 2024 21:39
@ashutosh-narkar
Copy link
Member

I didn't find any tests which verify the telemetry (trace) output

You can add tests here.

@@ -99,7 +101,13 @@ func Init(ctx context.Context, raw []byte, id string) (*otlptrace.Exporter, *tra
tlsOption,
)

attrs := make([]attribute.KeyValue, len(distributedTracingConfig.ResourceAttributes))
for key, value := range distributedTracingConfig.ResourceAttributes {
attrs = append(attrs, attribute.String(key, value))
Copy link
Member

Choose a reason for hiding this comment

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

Can resource attributes be any key value pair or do we need to do some validation here? For example, they must follow some naming convention, only some defined keys like service.namespace, service.version, service.name and service.instance.id can be used etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be any key-value pair. The opentelemetry spec defines some well-known resource attributes, but any key is allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note I've updated to allow a variety of data types for value (int, float, string, bool)

@brettmc
Copy link
Contributor Author

brettmc commented Aug 22, 2024

You can add tests here.

@ashutosh-narkar I didn't find those tests useful for testing this feature, since the code in distributedtracing.Init() creates a grpc exporter (which means I can't exercise my new code with tracetest's InMemoryExporter to inspect the generated spans).

I did try altering distributedtracing.Init() so that it could accept an exporter (which could be an in-memory exporter), but it looks like there is not a common interface between an in memory exporter and a grpc exporter...

if err == nil {
attrs = append(attrs, attribute.Float64(key, num))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we return an error if we can't convert to float64 or int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done.

@ashutosh-narkar
Copy link
Member

@brettmc I'm seeing unrelated changes included in your PR. Maybe something went wrong when you synced with main. Can you please look into that and squash and sign your commit. We can then get this in.

@brettmc brettmc force-pushed the otel-resource-attributes branch 2 times, most recently from 588b7a0 to ee1e25e Compare August 23, 2024 00:29
@brettmc
Copy link
Contributor Author

brettmc commented Aug 23, 2024

I was checking how other go-based apps with in-built OpenTelemetry do their config, and found https://docs.roadrunner.dev/docs/logging-and-observability/otel#configuration - this one only exposes a couple of the well-known attributes from the spec.

@ashutosh-narkar
Copy link
Member

I was checking how other go-based apps with in-built OpenTelemetry do their config, and found https://docs.roadrunner.dev/docs/logging-and-observability/otel#configuration - this one only exposes a couple of the well-known attributes from the spec.

I think that is a good approach to start. Just support the well-known ones for now.

@brettmc
Copy link
Contributor Author

brettmc commented Aug 24, 2024

I think that is a good approach to start. Just support the well-known ones for now.

Done. I think I'm happy with this PR now.

@@ -5296,6 +5296,23 @@ func TestDistributedTracingEnabled(t *testing.T) {
}
}

func TestDistributedTracingResourceAttributes(t *testing.T) {
c := []byte(`{"distributed_tracing": {
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to add "type": "grpc", here. Otherwise if you look at the Init method, this test currently is not exercising your new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Adding a resource map to the distributed_tracing
config. Entries in this map will be passed through to the OpenTelemetry SDK where they will be
added as resource attributes. The available resource attributes are service.namespace,
service.version and service.instance.id. see
https://opentelemetry.io/docs/specs/semconv/resource/

Fixes: open-policy-agent#6492

Signed-off-by: Brett McBride <brett@deakin.edu.au>
Signed-off-by: Brett McBride <brett@deakin.edu.au>
Signed-off-by: Brett McBride <brett@deakin.edu.au>
@ashutosh-narkar ashutosh-narkar merged commit fd0821c into open-policy-agent:main Aug 26, 2024
28 checks passed
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.

Add support for OpenTelemetry resource attributes
2 participants