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

[receiver/opencensus]: check for ErrListenerClosed when reporting multiplexer status #34093

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Jul 16, 2024

Fixes #33865

@odubajDT odubajDT force-pushed the correctness-traces branch 2 times, most recently from 7165db8 to c4ed6b9 Compare July 16, 2024 12:14
@odubajDT odubajDT changed the title [chore]: fix correctness traces [receiver/opencensus]: check for ErrListenerClosed when reporting multiplexer status Jul 16, 2024
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Two things:

  1. This should probably have a changelog since it's a bug fix.
  2. Is it possible to add a test for this? Maybe some way to make sure that shutting down isn't blocked in this specific case where the listener is closed?

@odubajDT
Copy link
Contributor Author

Two things:

1. This should probably have a changelog since it's a bug fix.

2. Is it possible to add a test for this? Maybe some way to make sure that shutting down isn't blocked in this specific case where the listener is closed?

I will try, but still testing if the actual fix resolves the issue, since I am not 100% sure yet, the PR is still in draft mode and chlog + some unit tests are still missing -> will add them ass soon as the PR will be ready for review+merging.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.86%. Comparing base (e495816) to head (c4ed6b9).
Report is 28 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #34093      +/-   ##
==========================================
- Coverage   82.97%   82.86%   -0.12%     
==========================================
  Files        2131     2132       +1     
  Lines      156561   157389     +828     
==========================================
+ Hits       129911   130421     +510     
- Misses      21927    22225     +298     
- Partials     4723     4743      +20     

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

@odubajDT odubajDT force-pushed the correctness-traces branch 2 times, most recently from 258e812 to 371ddc3 Compare July 18, 2024 07:38
@odubajDT odubajDT marked this pull request as ready for review July 18, 2024 08:26
@odubajDT odubajDT requested review from a team and mx-psi July 18, 2024 08:26
@mx-psi mx-psi requested a review from crobert-1 July 18, 2024 10:57
component: opencensusreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Check for ErrListenerClosed error before reporting multiplexer status.
Copy link
Member

Choose a reason for hiding this comment

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

This message should be a bit more user-friendly. What does the user of the opencensus receiver care about, and understand?

Maybe something like "Don't report an error for an expected condition during shutdown"?

receiver/opencensusreceiver/opencensus_test.go Outdated Show resolved Hide resolved
…tiplexer status

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@mx-psi mx-psi merged commit aab1424 into open-telemetry:main Jul 19, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: TestTracingGoldenData/otlp-opencensus timed out after 10m
4 participants