Upgrade opentelemetry deps from 0.26 -> 0.29 #907
Conversation
|
|
| global::set_error_handler(|err| { | ||
| tracing::error!("{}", err); | ||
| })?; |
There was a problem hiding this comment.
core/src/telemetry/otel.rs
Outdated
| let override_resource = Resource::builder().with_attributes(override_kvs).build(); | ||
| Resource::builder_empty() | ||
| .with_attributes( | ||
| default_resource_instance() | ||
| .iter() | ||
| .map(|(k, v)| KeyValue::new(k.clone(), v.clone())), | ||
| ) | ||
| .with_attributes( | ||
| override_resource | ||
| .iter() | ||
| .map(|(k, v)| KeyValue::new(k.clone(), v.clone())), | ||
| ) | ||
| .build() |
There was a problem hiding this comment.
open-telemetry/opentelemetry-rust#2322 👀
the removal of .merge is a bit of a bummer.
|
#908 is the real change I'm after. The next opentelemetry release (0.30) includes tonic 0.13 open-telemetry/opentelemetry-rust#2876 |
|
The removal of |
|
Thanks for trying this out. FYI I had started this before and had to abort because of another OTel bug that was causing the way we try to add global tags to fail: open-telemetry/opentelemetry-rust#2858 Unfortunately either we fix that upstream, or we work around that by manually applying global tags to every metric... which is obnoxious. #882 is the issue on our side |
Oh god, right, and this - there's just AFAICT literally no way to deal with this properly in current versions either: open-telemetry/opentelemetry-rust#2697 We might have to just accept that regression unfortunately, because, yeah, I don't want to never be able to upgrade. OTel upgrades are never the smoothest. |
upstream change made this functionality tricky open-telemetry/opentelemetry-rust#2697 open-telemetry/opentelemetry-rust#2260
|
@Sushisource would you mind kicking workflows one more time if you get a chance?
was there a failing integration test for this? I think I got integration tests running and didn't see anything glaring wrong with global tags. this is the only local failure I see with the integration tests: pretty bizarre. it seems like the I ran:
I'm not sure if there is another step I was missing |
|
Strange, not sure what that one's about. Also I thought we had these configured so that I only had to approve once but maybe that changed. There's a handful of unrelated flakes I need to find some time to deal with too. And no -- there's no test for the global tags. I do have this branch though that demonstrates what I needed to change to get them to work (they're not really working properly now anyway), but that making that change, which should work, doesn't - so with that in mind we don't need to block this upgrade on it. I was just delaying things hoping it might get fixed. |
|
Thanks for this! I realized I think I have a solution to the global error logging problem, if you don't want to take this all on (and I totally get it if not) let me know and I'll try to find some time to add it to this PR, but this is a solution I think will work fine: We can use https://docs.rs/tracing/latest/tracing/subscriber/fn.set_global_default.html which I didn't appreciate can act like a fallback. It can just use the standard console formatting from here with a simple broad error-level-only filter. Then, setting that will need to be opt-in (so we don't bother any Rust users using Rust SDK or Core directly who want to set their own global default). So, we can just have some top-level function in the telemetry mod like As for the test, somewhat annoyingly it might need to be moved out into its own totally separate integration test module so that it can call that function and run in it's own process and not muck up any other tests. |
What was changed
This change upgrades opentelemetry deps from 0.26 -> 0.29.
Why?
This change is a bit of yak shaving to prep for upgrading tonic 0.13
Checklist
Closes [Feature Request] Update to OpenTelemetry 0.27 #862
How was this tested:
Modified entries in Cargo.toml -> ran
cargo test-> fixed compile errors -> rancargo testNo