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

Fix race on Collector.Start() loosing error info #255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iximiuz
Copy link

@iximiuz iximiuz commented Jul 2, 2022

When the inner collector (go.opentelemetry.io/collector/service) Run() call fails
(likely due to a bad config file) the lambda collector wrapper would
likely loose the actual error information and report just the unexpected
collector state (CLOSED) back to the caller. This PR makes the collector wrapper's
Start() method race-free, so that the actual error information is not
lost.

Another improvement is related to the main() procedure - before this PR the main loop
wouldn't handle situations when the inner collector exits before the
SHUTDOWN event.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 2, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: iximiuz / name: Ivan Velichko (90898ef)

@iximiuz iximiuz force-pushed the iximiuz/fix-collector-start-race branch 2 times, most recently from a34c27c to c5e9f1c Compare July 2, 2022 12:49
When the inner collector (go.opentelemetry.io/collector/service) Run() call fails
(likely due to a bad config file) the lambda collector wrapper would
likely loose the actual error information and report just the unexpected
collector state (CLOSED) back to the caller. This PR makes the collector wrapper's
Start() method race-free, so that the actual error information is not
lost.

Another improvement is related to the main() procedure - before this PR the main loop
wouldn't handle situations when the inner collector exits before the
SHUTDOWN event.
@iximiuz iximiuz force-pushed the iximiuz/fix-collector-start-race branch from c5e9f1c to 90898ef Compare July 2, 2022 13:03
@NathanielRN
Copy link
Contributor

@Aneurysm9 could you please help take a look at this? I remember last time we edited these files in #194 we were burned pretty bad before a release until we fixed in #207 😓 Please let me know what you think of this change!

@iximiuz
Copy link
Author

iximiuz commented Aug 26, 2022

Kind reminder that this PR is still awaiting its review 😇

@Aneurysm9
Copy link
Member

I think this PR will not cause the same issues that #194 did. It does not hang the initialization until the end of svc.Run() like that one did. I will try to run this through some tests today, but the approach and code seem sound to me.

@NathanielRN
Copy link
Contributor

Thank you @Aneurysm9! I defer to you to merge if you think it looks good 🙂

@tylerbenson
Copy link
Member

Is this still a problem? This is a pretty old PR. I'm inclined to close it unless you want to revisit/rebase on main.

@iximiuz
Copy link
Author

iximiuz commented Dec 15, 2023

@tylerbenson feel free to close it. I won't have time to check if the issue remains, and we don't use this code in prod now.

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.

None yet

5 participants