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

Access to Collector Log Pointer is not atomic #4939

Closed
cpheps opened this issue Mar 2, 2022 · 7 comments
Closed

Access to Collector Log Pointer is not atomic #4939

cpheps opened this issue Mar 2, 2022 · 7 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Good issue for contributors to OpenTelemetry Service to pick up priority:p2 Medium

Comments

@cpheps
Copy link
Contributor

cpheps commented Mar 2, 2022

Describe the bug
Access to the collectors log pointer is not atomic. If GetLogger is called before Run the returned pointer is no longer valid as it is updated during startup.

It also presents a race condition between calling shutdown and run as shutdown could reference the pointer in the recover before or after it's set.

Steps to reproduce

  1. Call GetLogger before Run
  2. Call Run
  3. Observe GetLogger is still using the noop logger

What did you expect to see?
If I call GetLogger before Run the pointer I'm given should update and log.

What did you see instead?
Log pointer is not atomic so GetLogger will return a different point before and after Run is called.

What version did you use?
v0.45.0

What config did you use?
Any will work

Environment
OS: MacOS 12.2.1
Compiler(if manually compiled): go 1.17.6

Additional Context

Action item from #4827

@patil-kshitij
Copy link
Contributor

Can I start looking into this ?

@cpheps
Copy link
Contributor Author

cpheps commented Mar 2, 2022

@patil-kshitij by all means. I might have capacity to do so until next week so feel free.

@dmitryax
Copy link
Member

cc @djaglowski

@sakshi1215
Copy link
Contributor

Hey @dmitryax @cpheps Can you please assign this issue to me, I would like to take a dig at this.

@dmitryax
Copy link
Member

dmitryax commented Oct 10, 2022

Thanks @sakshi1215. Assigned

@sakshi1215
Copy link
Contributor

I was looking at this issue & have noticed that its no longer valid. Since version 0.54 we have remove the GetLogger method as well the Collector object no longer packagers Logger instance with it but only has LoggerOption under set.LoggingOptions
Reference:

set CollectorSettings

https://github.com/open-telemetry/opentelemetry-collector/blob/main/CHANGELOG.md#-breaking-changes--7

Can we close this @dmitryax & @cpheps ?

@dmitryax
Copy link
Member

Thanks @sakshi1215. Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Good issue for contributors to OpenTelemetry Service to pick up priority:p2 Medium
Projects
None yet
Development

No branches or pull requests

5 participants