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

[Serverles] fix metrics multiline logs #11335

Merged
merged 3 commits into from
Mar 21, 2022
Merged

Conversation

maxday
Copy link
Contributor

@maxday maxday commented Mar 17, 2022

What does this PR do?

Debug log lines have been added in this PR. Originally, metrics were output in one single line.
Due to a refactor here, SketchSeriesList was moved to in internal package and .String() has become unavailable.

This PR:

  • moves String() out of the internal package
  • explicitely calls .String() in the debug log line to prevent future failures
  • adds a unit test

Before the fix:
Screen Shot 2022-03-17 at 2 20 41 PM
After:
Screen Shot 2022-03-17 at 2 20 26 PM

Describe how to test/QA your changes

Unit test

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
  • A release note has been added or the changelog/no-changelog label has been applied.
  • Changed code has automated tests for its functionality.
  • Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
  • At least one team/.. label has been applied, indicating the team(s) that should QA this change.
  • If applicable, docs team has been notified or an issue has been opened on the documentation repo.
  • If applicable, the need-change/operator and need-change/helm labels have been applied.
  • If applicable, the config template has been updated.

@maxday maxday added this to the Triage milestone Mar 17, 2022
@maxday maxday requested a review from a team as a code owner March 17, 2022 18:35
go.sum Outdated
@@ -115,6 +115,7 @@ github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03
github.com/BurntSushi/toml v0.4.1 h1:GaI7EiDXDRfa8VshkTj7Fym7ha+y8/XxIgD2okUIjLw=
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this go.sum update from the pull-request?
I think it may be because you have extra/past work done in your directory already which is not yet merged to main or sticked into your go.sum? Not sure. But this is probably better to not commit it as-is as there is many deps changes in there unrelated to your actual fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oopsy that's definitely a mistake from my end, this file was not intended to be committed! thanks!

@maxday maxday requested a review from remeh March 18, 2022 18:01
Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

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

LGTM!

@maxday maxday merged commit 930c100 into main Mar 21, 2022
@maxday maxday deleted the maxday/fix-metric-logs-in-one-line branch March 21, 2022 13:49
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.

None yet

3 participants