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

chore: better logging for config state and transaction sampling #4291

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

trentm
Copy link
Member

@trentm trentm commented Oct 31, 2024

This adds a periodic (once per minute) dump of effective config changes
from the default, once per minute at 'trace'-level.

This also fixes an issue where a central-config update of log_level
did not update the child logger on the APM client. Only the agent's
own logger's level was updated.

This adds trans.sampled to some debug logging output.


@david-luna Note this is targeting 3.x. If this looks reasonable, I'll adapt the patch for "main" as well.

This adds a periodic (once per minute) dump of effective config changes
from the default, once per minute at 'trace'-level.

This also fixes an issue where a central-config update of `log_level`
did not update the *child* logger on the APM client. Only the agent's
own logger's level was updated.

This adds trans.sampled to some debug logging output.
@trentm trentm self-assigned this Oct 31, 2024
@trentm
Copy link
Member Author

trentm commented Oct 31, 2024

I need to work through the test errors.

@trentm
Copy link
Member Author

trentm commented Nov 2, 2024

The v20 test failures are fixed by #4295

// agent by setting trace-level logging and getting 1 minute of logs.
// (Sometimes getting logs from application *start* is no possible.)
setInterval(() => {
if (this.logger.isLevelEnabled('trace')) {
Copy link
Member

Choose a reason for hiding this comment

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

could the level be changed at runtime? If not I think we could setInterval within this block.

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 it can be changed at runtime via central config, and in the use case I needed this for that was the case.
I balked at integrating this change with central config handling.

However, I may do that for the main branch version of this change, because having a setInterval running all the time kinda sucks. On the other hand, having a setInterval (change in behaviour, however slight) that only turns on in "trace" made could mean more subtle bugs. It would be awful if there was a crash that only resulted from code that enabled when in "trace" mode. I'm talking generally.

@david-luna
Copy link
Member

@david-luna Note this is targeting 3.x. If this looks reasonable, I'll adapt the patch for "main" as well.

makes sense to have record of the diif in config and also the sampled transactions. This is main material :)

@trentm trentm merged commit af926eb into 3.x Nov 4, 2024
30 of 32 checks passed
@trentm trentm deleted the trentm/trans-sampled-logging branch November 4, 2024 16:27
@trentm trentm mentioned this pull request Nov 4, 2024
trentm added a commit that referenced this pull request Nov 4, 2024
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.

2 participants