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

Draft: Add OpenTelemetry #4345

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Draft: Add OpenTelemetry #4345

wants to merge 7 commits into from

Conversation

vooon
Copy link
Contributor

@vooon vooon commented Jul 5, 2021

What is this change?

Adds distributed tracing with OpenTelemetry.

Why is this change necessary?

It's very handy tool to debug what's going on in your distributed service.

Does your change need a Changelog entry?

Yes.

Do you need clarification on anything?

  • Need to find how to test store/etcd context expectations.
  • Need to clarify span naming.

Were there any complications while making this change?

  • It's impossible to make nested traces for message buses - no context.
  • Perhaps no need to trace watcher-loops.

Have you reviewed and updated the documentation for this change? Is new documentation required?

  • Perhaps need to update configuration with new options.

How did you verify this change?

  • It is being used for several month.

Is this change a patch?

No.

@vooon vooon force-pushed the add-otel branch 2 times, most recently from 2709895 to 403c0be Compare July 8, 2021 11:47
vooon added 6 commits July 8, 2021 14:50
Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
- add tracer to apid
- add tracer to etcd store
- add tracer to etcdstore v2
- add tracer to event_store
- add tracers to schedulerd
- add tracers to eventd
- add tracers to cache
- add tracer to health_store
- add tracer to pipeline
- add traces to ringv2
- add span to ringv2 hasTrigger

Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
@echlebek
Copy link
Contributor

Very interesting. I see the PR is in draft status, do you intend to work on it further?

@vooon
Copy link
Contributor Author

vooon commented Jul 14, 2021

@echlebek yes, but i have two blokers for now:

  1. 83d32a1 - i disabled that tests because they fails on resource context comparison (context.Background() != context.WithValue(context.Background(), <otel-thing>)). Need to find how to check that base context is the same with ignore for value-nesting.
  2. Need to wait for OTEL 1.0.0, update etcd, update PR: Update OpenTelemetry to 1.0.0 etcd-io/etcd#13141

@vooon
Copy link
Contributor Author

vooon commented Sep 21, 2021

@echlebek OTEL 1.0.0 have been released. But that'll still be a problem because etcd 3.6 far from release as i know.

@vooon
Copy link
Contributor Author

vooon commented Oct 20, 2021

@echlebek etcd main now has fresh OTEL, so now i can rebase this PR. But i would have to replace etcd version.

@echlebek
Copy link
Contributor

@fguimond in light of your recent spike work on open telemetry, do you think this PR can be partially incorporated?

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.

2 participants