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

[otelcol] rfc for how to log during startup #10066

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented May 1, 2024

Description

This is an RFC to help us decide how we want otelcol to provide a logger before the primary logger is created. As we discuss I will update the doc. Before this is merged we should have decided on a solution and the Accepted Solution section must be updated.

Related to #10056

Link to tracking issue

This unblocks:

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 1, 2024
Copy link
Contributor

@evan-bradley evan-bradley 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 writing this up.

docs/rfcs/logging-before-config-resolution.md Outdated Show resolved Hide resolved
docs/rfcs/logging-before-config-resolution.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.84%. Comparing base (d4d2d9a) to head (59d8173).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10066      +/-   ##
==========================================
- Coverage   91.87%   91.84%   -0.03%     
==========================================
  Files         360      361       +1     
  Lines       16725    16722       -3     
==========================================
- Hits        15366    15359       -7     
- Misses       1021     1025       +4     
  Partials      338      338              

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

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I'm in favour of the first option, buffer the logs until we either get a configured logger, or emit them over stderr on the case of the configuration failing to resolve.

One question here is what happens in the case of the configuration taking a long time to resolve (if someone is using a remote, ie s3, provider)?

docs/rfcs/logging-before-config-resolution.md Outdated Show resolved Hide resolved
docs/rfcs/logging-before-config-resolution.md Show resolved Hide resolved
@evan-bradley
Copy link
Contributor

One question here is what happens in the case of the configuration taking a long time to resolve (if someone is using a remote, ie s3, provider)?

That's a good question. I think unfortunately users will just see a delay before the logs are printed on Collector startup. I think this is an acceptable tradeoff considering the Collector is usually started without a human looking at the console.

@TylerHelmuth
Copy link
Member Author

I've updated #10056 to implement solution 1 and all solution requirements as an example implementation

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

From #10008 (comment), can we list whether preserving the following is required?

  1. The timestamp
  2. The EntryCaller (what line number the log originates from)
  3. The 'terminal behavior' (whether to panic at panic level)

I think there is consensus about preserving (1), I think (2) would be nice, and I personally don't care much about (3) (we don't really use that at all, and we tell people not to panic at runtime)

@TylerHelmuth
Copy link
Member Author

@mx-psi preserving 1 is definitely required (and called out in the rfc). I think 2 should be required as well, if we all agree I can add that. 3 I don't think needs to be preserved as panicing goes against our development principals.

@mx-psi
Copy link
Member

mx-psi commented May 3, 2024

@mx-psi preserving 1 is definitely required (and called out in the rfc). I think 2 should be required as well, if we all agree I can add that. 3 I don't think needs to be preserved as panicing goes against our development principals.

Oki, that works for me. A solution with a changeable zapcore.Core should work here then, and we won't need to copy any internals if we don't require (3)

@TylerHelmuth
Copy link
Member Author

@codeboten @mx-psi @djaglowski @evan-bradley I've updated the RFC with an accepted solution based on the PRs discussion.

docs/rfcs/logging-before-config-resolution.md Outdated Show resolved Hide resolved
docs/rfcs/logging-before-config-resolution.md Outdated Show resolved Hide resolved
docs/rfcs/logging-before-config-resolution.md Outdated Show resolved Hide resolved
@mx-psi mx-psi requested a review from evan-bradley May 6, 2024 15:20
@codeboten codeboten merged commit 227101d into open-telemetry:main May 6, 2024
47 checks passed
@github-actions github-actions bot added this to the next release milestone May 6, 2024
@TylerHelmuth TylerHelmuth deleted the rfc-collector-logging-before-config-resolution branch May 6, 2024 16:18
andrzej-stencel pushed a commit to andrzej-stencel/opentelemetry-collector that referenced this pull request May 27, 2024
#### Description

This is an RFC to help us decide how we want `otelcol` to provide a
logger before the primary logger is created. As we discuss I will update
the doc. Before this is merged we should have decided on a solution and
the Accepted Solution section must be updated.

Related to
open-telemetry#10056

#### Link to tracking issue
This unblocks:
- open-telemetry#9162
- open-telemetry#5615

---------

Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants