extension: Add Span::setTag for setting attributes in OTLP Tracer #21893
Conversation
Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
Hi @htuch! When this auto-added you for review, I realized I added you for this extension under CODEOWNERS in the last PR without checking with you — happy to update that if you'd like, as I'm sure you get plenty of review requests already :) |
|
@AlexanderEllis initial testing and it looks like this is working well. Fantastic! And thanks for working on this |
|
Awesome, glad to hear it, and thanks for testing it out! |
|
One thing I'm stuck on is the health check filter. I have a cluster health check on my front Envoy "health_checks": [
{
"always_log_health_check_failures": true,
"healthy_threshold": 2,
"http_health_check": {
"path": "/z/",
"codec_client_type": "HTTP2",
"service_name_matcher": {
"exact": "sync_service"
}
},
"interval": "5s",
"interval_jitter": "0.5s",
"timeout": "1s",
"unhealthy_threshold": 2
}
],and then a health check filter on the upstream Envoy {
"name": "envoy.filters.http.health_check",
"typed_config": {
"@type": "type.googleapis.com/envoy.extensions.filters.http.health_check.v3.HealthCheck",
"headers": [
{
"name": ":path",
"string_match": {
"exact": "/z/"
}
}
],
"pass_through_mode": false
}
},But the OpenTelemetry Collector is still seeing and sending health checks with Feels like this should be handled in the HCM. I'm pretty sure the filter worked when using the Lightstep tracer previously. Any ideas? |
|
LGTM, but we probably should have a non-Googler maintainer consistently reviewing these as well, @mattklein123 can you take a look at this short PR as you also reviewed the main OT extension? I'm happy to remain CODEOWNERS, but it's possible @yanavlasov or someone else in our Envoy Platform team might take it if they have cycles. |
|
@moderation Thanks for calling that out! Just to make sure I'm on the same page, you have your front Envoy health checking your upstream Envoy, and the OTel collector is not seeing the traces from that check with the right user agent? I'm going to be afk this week, but I should be able to dig in and try that setup out myself after the 4th 👍 Edit: ah I think I may see what's going on: you're seeing the traces, but you don't want to, since it's just a health check. From a quick check, I think this is because this new OTel tracer isn't checking each Span's @htuch Sounds great to me, thanks! |
| // Attribute keys MUST be unique. | ||
| // If a value already exists for this key, overwrite it. | ||
| for (auto& key_value : *span_.mutable_attributes()) { | ||
| if (key_value.key() == name) { | ||
| key_value.mutable_value()->set_string_value(std::string{value}); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
This kind of N^2 behavior seems sub-optimal, though I guess there aren't usually that many tags. It might be worth it to optimize this later.
|
@AlexanderEllis yep your edit is exactly right. The health checks are being traced. A check like the Zipkin tracer should fix this. Thanks again |
…voyproxy#21893) Add Span::setTag for setting attributes on OTLP span Signed-off-by: Alex Ellis <ellisonjtk@gmail.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
|
@moderation Should be all set with #22047 merged in 👍 Thanks again for trying this out! |
Signed-off-by: Alex Ellis ellisonjtk@gmail.com
Commit Message: Add Span::setTag for setting attributes in OTLP Tracer
Additional Description: Adding attributes to the OTLP traces generated in the OTLP tracer (see #9958).
Here's a sampling of the attributes Envoy creates in a basic test setup (Envoy making a child span for its call to the Nighthawk test server) by just adding this method:
Risk Level: low
Testing: Added a unit test (with fleshed out proto expectations)
Docs Changes: N/A
Part of #9958