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

Add few debug level internal logs to PeriodicReader #2277

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cijothomas
Copy link
Member

Debug level, only for troubleshooting deep.
Need to find a common style like whether every Message should end with a period etc. in the future.

@cijothomas cijothomas requested a review from a team as a code owner November 5, 2024 23:03
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.5%. Comparing base (dd7b531) to head (c4ef0e8).

Files with missing lines Patch % Lines
opentelemetry-sdk/src/metrics/periodic_reader.rs 92.8% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2277     +/-   ##
=======================================
- Coverage   79.5%   79.5%   -0.1%     
=======================================
  Files        121     121             
  Lines      20960   20974     +14     
=======================================
+ Hits       16673   16684     +11     
- Misses      4287    4290      +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -251,6 +257,10 @@ impl<RT: Runtime> PeriodicReaderWorker<RT> {
async fn process_message(&mut self, message: Message) -> bool {
match message {
Message::Export => {
otel_debug!(
name: "PeriodicReader.ExportMessageReceived",
Copy link
Contributor

Choose a reason for hiding this comment

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

Message and channels are implementation related terms. I think we should use more generic and intuitive terms for event names such as these:

  • PeriodicReader.ExportTriggered
  • PeriodicReader.ForceFlushCalled
  • PeriodicReader.ShutdownCalled

Copy link
Member

@lalitb lalitb Nov 6, 2024

Choose a reason for hiding this comment

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

Not related to this comment, but just wondering if it should be fine to include implementation-specific terms in debug macros, as they're not intended for the end user?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think its inevitable that we need to use implementation specific terms in some places. My only goal is to restrict them to TRACE/DEBUG level logs only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use user-friendly names wherever possible for any log level? It would make the collected logs more readable. It would also save the effort of renaming event names, if at all we change the underlying implementation details.

We can use implementation details in the event name, when it's indeed inevitable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's the plan.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is just adding some debug logs, so I can see some logs without having to "trigger" an error condition to see a log, and see if self-diag example can be removed. (#2274 (comment))

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.

3 participants