-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Improve connection state logging for Jaeger exporter #2239
Improve connection state logging for Jaeger exporter #2239
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2239 +/- ##
==========================================
+ Coverage 91.90% 91.92% +0.02%
==========================================
Files 269 270 +1
Lines 15239 15295 +56
==========================================
+ Hits 14005 14060 +55
- Misses 855 856 +1
Partials 379 379
Continue to review full report at Codecov.
|
de4901c
to
e4d2278
Compare
e4d2278
to
a83f04e
Compare
e9a0d08
to
f33f064
Compare
@bogdandrutu would you please review it again? |
It would be great to make this work for all gRPC exporters. Not necessarily in this PR, but in the future. |
Sure, let's get this one merged and see the feedback from users. I'll then refactor the main logic from this PR into a generic facility, to be used by gRPC-based exporters. |
ping @bogdandrutu |
1 similar comment
ping @bogdandrutu |
@bogdandrutu @tigrannajaryan ping? |
exporter/jaegerexporter/exporter.go
Outdated
} | ||
s.AddStateChangeCallback(s.onStateChange) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever more than one callback needed? Also, perhaps initialize it in the above struct initialization. It is not clear why this gets a special treatment while other fields are initialized directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever more than one callback needed?
No, perhaps in tests only. In any case, I think it's a good general pattern to allow multiple callbacks to exist. I'm happy to make it a simple property if you prefer.
It is not clear why this gets a special treatment while other fields are initialized directly.
It's because it needs to refer the instance s
.
I won't be able to work on the review comments until I'm back from the holiday break (Jan 10). |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
f33f064
to
dea3b2f
Compare
PR updated. There's only one pending decision, about the callbacks. |
@tigrannajaryan do you know what's wrong with the CI? Can you start it again? |
745f47c
to
a12ca44
Compare
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
a12ca44
to
1632b15
Compare
CI is failing with an unrelated issue:
|
ping @bogdandrutu , @tigrannajaryan |
Bumps [lycheeverse/lychee-action](https://github.com/lycheeverse/lychee-action) from 1.5.1 to 1.5.2. - [Release notes](https://github.com/lycheeverse/lychee-action/releases) - [Commits](lycheeverse/lychee-action@v1.5.1...v1.5.2) --- updated-dependencies: - dependency-name: lycheeverse/lychee-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description:
Adds a connection state reporter to the Jaeger exporter sending state changes to the log and as a metric.
Link to tracking Issue: Closes #1861, as it's the best we can do at this exporter level. On a more general level, the collector could provide more verbose info about the underlying gRPC connections. See #2237 for more information.
Testing: unit and manual tests.
Documentation: None.
Here are the complete logs for an OpenTelemetry Collector that got started without a Jaeger backend available. Note that the config.yaml contains two exporters, one (
jaeger
) with theinsecure: true
option, and a second one (jaeger/2
) without this option. A Jaeger all-in-one is then started, without TLS support. This ends with the first exporter eventually transitioning to "READY", but the second won't.And here are the metrics for the final state:
Signed-off-by: Juraci Paixão Kröhling [email protected]