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 panic in labels debug message #1687

Merged
merged 1 commit into from
Feb 17, 2020
Merged

Conversation

adityacs
Copy link
Collaborator

@adityacs adityacs commented Feb 13, 2020

Just a placeholder for now. Need to test more

What this PR does / why we need it:
Fix panic in labels debug message

Which issue(s) this PR fixes:
Fixes #1680

@codecov-io
Copy link

codecov-io commented Feb 13, 2020

Codecov Report

Merging #1687 into master will increase coverage by 0.79%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1687      +/-   ##
==========================================
+ Coverage   63.27%   64.07%   +0.79%     
==========================================
  Files         121      121              
  Lines        9027     9027              
==========================================
+ Hits         5712     5784      +72     
+ Misses       2897     2841      -56     
+ Partials      418      402      -16
Impacted Files Coverage Δ
pkg/logentry/stages/timestamp.go 87.34% <100%> (+10.12%) ⬆️
pkg/logentry/stages/regex.go 78.57% <100%> (+17.85%) ⬆️
pkg/logentry/stages/output.go 75.75% <100%> (+18.18%) ⬆️
pkg/logentry/stages/tenant.go 95.12% <100%> (+14.63%) ⬆️
pkg/logentry/stages/template.go 86.66% <100%> (+17.77%) ⬆️
pkg/logentry/stages/labels.go 78.94% <100%> (+10.52%) ⬆️
pkg/logentry/stages/metrics.go 59.89% <66.66%> (+7.69%) ⬆️
pkg/promtail/targets/tailer.go 75.86% <0%> (-2.3%) ⬇️
pkg/ingester/transfer.go 66.42% <0%> (+1.42%) ⬆️
... and 10 more

Copy link
Member

@owen-d owen-d 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 tracking this down! I agree with @cyriltovena, lets add a small test for this then LGTM.

@adityacs
Copy link
Collaborator Author

adityacs commented Feb 14, 2020

@owen-d @cyriltovena need a small clarity. Just wanted to know what's the take here. Should we just remove .String() from reflect.TypeOf(lValue).String() in all Debug lines ?
OR
should we set extracted[n] = "" here https://github.com/grafana/loki/blob/master/pkg/logentry/stages/json.go#L153 ?

@cyriltovena
Copy link
Contributor

I think removing the .String() is a good idea. Pretty sure the logger will be fine, but the best option is to have a test covering this line/case.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promtail: Nil pointer reference, when parsing JSON logs
4 participants