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.

Sign up for GitHub

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

Jump to bottom

feat(trait): Add telemetry trait #3970

Merged
merged 2 commits into from
Jan 18, 2023
Merged

feat(trait): Add telemetry trait #3970

merged 2 commits into from
Jan 18, 2023

Conversation

gansheer
Copy link
Contributor

@gansheer gansheer commented Jan 10, 2023 •
edited
Loading

Resolves: #3519

Motivation

Add a new addon/trait telemetry to provide new default way for tracing. The current tracing trait based on opentracing is deprecated both in camel-quarkus and quarkus and will be deprecated.

Description

The new telemetry trait has been added with the basic configuration:

  • enable : disabled by default
  • auto : Jaeger discovery for OTLP API (more can be added with OTLP compatibility like Opentelemetry or Grafana Tempo)
  • service-name : default to integration name
  • sampler: default to on (alwaysOn), other values from OTLP protocol documentation
  • sampler-ratio: only used in case of ratio sampler
  • sampler-parent-based: if sampler is parent based

Existing e2e test using tracing have been updated to use telemetry.

Next step : deprecate tracing trait

Runtime : Linked PR with camel-k-runtime update: apache/camel-k-runtime#945

Release Note

feat(trait): Add telemetry trait as the new default tracing feature.

tadayosi
tadayosi reviewed Jan 11, 2023
View reviewed changes
Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

Only nitpick comments but otherwise looks good to me.

addons/telemetry/telemetry.go Outdated Show resolved Hide resolved
docs/modules/ROOT/nav.adoc Show resolved Hide resolved
@gansheer gansheer force-pushed the feature/3519_tracing_opentelemetry branch 2 times, most recently from de84ac8 to fcf5b94 Compare January 11, 2023 08:39
squakez
squakez approved these changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Nice work!

addons/tracing/tracing.go
//
// The trait is able to automatically discover the tracing endpoint available in the namespace (supports **Jaeger**).
//
// The Tracing trait is disabled by default.
//
// WARNING: The Tracing trait can't be enabled at the same time as the Telemetry trait.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd take the opportunity and add a deprecation notice at least here, reminding to use Telemetry trait.

docs/modules/traits/pages/telemetry.adoc

|===

// End of autogenerated code - DO NOT EDIT! (configuration)
Copy link
Contributor

Choose a reason for hiding this comment

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

After this autogenerated code (or even in the trait comment section) we could add some example how to use the trait. We did in other traits if you want to have a look https://camel.apache.org/camel-k/1.11.x/traits/toleration.html#_examples

@gansheer gansheer mentioned this pull request Jan 11, 2023
Open
@gansheer gansheer force-pushed the feature/3519_tracing_opentelemetry branch 4 times, most recently from 25ea619 to ae51de1 Compare January 11, 2023 17:37
squakez
squakez approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Very nice job! +1 for the new telemetry test suite, it opens the door to further refined testing on observability. Let us know when this is ready to merge as for me it looks already great!

@gansheer
Copy link
Contributor Author

gansheer commented Jan 12, 2023 •
edited
Loading

Very nice job! +1 for the new telemetry test suite, it opens the door to further refined testing on observability. Let us know when this is ready to merge as for me it looks already great!

I need to actually add the actual testing code (with trace verification) before 😅 , that's why it is still a draft.

@gansheer gansheer force-pushed the feature/3519_tracing_opentelemetry branch 17 times, most recently from 096802b to 08e463b Compare January 16, 2023 18:15
@gansheer gansheer marked this pull request as ready for review January 17, 2023 08:02
@gansheer
Copy link
Contributor Author

@squakez it's ready on my side. Don't hesitate to take the time to check the e2e test.

@gansheer gansheer mentioned this pull request Jan 17, 2023
Merged
squakez
squakez approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Some little possible changes to consider, but overall is a great job.

e2e/support/test_support.go Outdated
@@ -445,6 +445,17 @@ func IntegrationLogs(ns, name string) func() string {
}
}

func TailedLogs(ns, name string) func() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should parametrize the number of lines to tail IMO. It is a quick win very useful for future usage. Additionally some quick comment as a documentation would be nice as well.

e2e/support/test_support.go Outdated
ctrl.MatchingLabels{
"app.kubernetes.io/name": appName,
}); err != nil {
failTest(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to return the error and let the invoker to fail or do any other action instead of failing here.

@gansheer gansheer force-pushed the feature/3519_tracing_opentelemetry branch from 08e463b to d69ea5c Compare January 17, 2023 12:12
@gansheer
feat(trait): Add telemetry trait
16a0c02 
* Define telemetry addon
* Jaeger discovery for OTLP API
* Basic configuration for tracing OTLP sampler
* update existing e2e using tracing to telemetry

Resolves: apache#3519
@gansheer gansheer force-pushed the feature/3519_tracing_opentelemetry branch 2 times, most recently from c78c16f to 525bc53 Compare January 17, 2023 19:54
@gansheer
feat(e2e): Add Telemetry e2e test
c210a98 
* Add specific github action
* Add opentelemetry basic config collector in telemetry e2e env
@gansheer gansheer force-pushed the feature/3519_tracing_opentelemetry branch from 525bc53 to c210a98 Compare January 17, 2023 22:02
@squakez squakez merged commit 69d8d73 into apache:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tadayosi tadayosi tadayosi approved these changes

@oscerd oscerd oscerd approved these changes

@squakez squakez squakez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move tracing from opentracing to opentelemetry
4 participants
@gansheer @tadayosi @oscerd @squakez