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.

Sign up for GitHub

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

Jump to bottom

Promtail: add static labels in stage #4495

Merged
merged 1 commit into from
Oct 26, 2021
Merged

Promtail: add static labels in stage #4495

merged 1 commit into from
Oct 26, 2021

Conversation

sankalp-r
Copy link
Contributor

@sankalp-r sankalp-r commented Oct 19, 2021 •
edited

Signed-off-by: Sankalp Rangare sankalprangare786@gmail.com

What this PR does / why we need it:
This PR adds a new stage in Promtail called static_labels to provide custom labels.

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

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@sankalp-r sankalp-r requested a review from a team as a code owner October 19, 2021 13:49
@CLAassistant
Copy link

CLAassistant commented Oct 19, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@sankalp-r
Copy link
Contributor Author

@dannykopping I am not able to figure out if build failed because of my changes.

@dannykopping
Copy link
Contributor

Restarted the build, let's see

@sankalp-r
Copy link
Contributor Author

Restarted the build, let's see

@dannykopping Can you please review it?

@dannykopping
Copy link
Contributor

Restarted the build, let's see

@dannykopping Can you please review it?

Yes, I will review it as soon as I have some time - it may take a few days unfortunately.

cyriltovena
cyriltovena reviewed Oct 25, 2021
View reviewed changes
clients/pkg/logentry/stages/static_labels.go
return errors.New(ErrEmptyStaticLabelStageConfig)
}
for labelName, labelSrc := range c {
if !model.LabelName(labelName).IsValid() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also validate the label value ?

Copy link
Contributor Author

@sankalp-r sankalp-r Oct 25, 2021 •
edited

Choose a reason for hiding this comment

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

I have added label-value validation in Process() function.

cyriltovena
cyriltovena reviewed Oct 25, 2021
View reviewed changes
clients/pkg/logentry/stages/static_labels.go Outdated
}
// If no label source was specified, use the key name
if labelSrc == nil || *labelSrc == "" {
lName := labelName
Copy link
Contributor

Choose a reason for hiding this comment

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

What;s the use case ?

Copy link
Contributor Author

@sankalp-r sankalp-r Oct 25, 2021 •
edited

Choose a reason for hiding this comment

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

I just took the idea from labels.go.
If no label-value is specified, we can just use the label-name as label-value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this explicit. If folks want a label/value combination of the same value, they should define it as such.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this @sankalp-r

Copy link
Contributor Author

@sankalp-r sankalp-r Oct 26, 2021 •
edited

Choose a reason for hiding this comment

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

@dannykopping How should we handle the case when labelSrc is nil ?
It will cause error if we try to access nil value in Process function.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should skip the label in that case IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dannykopping Added skip logic in the Process function.

cyriltovena
cyriltovena reviewed Oct 25, 2021
View reviewed changes
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.

Looking good !

can't you answer my feedbacks and also introduce the new documentation section required for this.

Thanks !

@dannykopping dannykopping self-assigned this Oct 25, 2021
@sankalp-r sankalp-r force-pushed the allow-static-label branch 2 times, most recently from 1a83bac to 59fcecf Compare October 26, 2021 06:00
@sankalp-r
Copy link
Contributor Author

Looking good !

can't you answer my feedbacks and also introduce the new documentation section required for this.

Thanks !

I have added documentation section. Please have a look.

dannykopping
dannykopping suggested changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

@sankalp-r this looks great, thanks for your contribution!

I've added a few minor spelling corrections, and added to the question asked by Cyril. Once we have all those fixed up this one will be good to merge 👍

clients/pkg/logentry/stages/stage.go Outdated Show resolved Hide resolved
clients/pkg/logentry/stages/stage.go Outdated Show resolved Hide resolved
clients/pkg/logentry/stages/static_labels.go Outdated
}
// If no label source was specified, use the key name
if labelSrc == nil || *labelSrc == "" {
lName := labelName
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this explicit. If folks want a label/value combination of the same value, they should define it as such.

clients/pkg/logentry/stages/static_labels.go Outdated Show resolved Hide resolved
clients/pkg/logentry/stages/static_labels.go Outdated Show resolved Hide resolved
docs/sources/clients/promtail/stages/static_labels.md Outdated Show resolved Hide resolved
@sankalp-r
Copy link
Contributor Author

@sankalp-r this looks great, thanks for your contribution!

I've added a few minor spelling corrections, and added to the question asked by Cyril. Once we have all those fixed up this one will be good to merge +1

Done the changes, please have a look.

dannykopping
dannykopping reviewed Oct 26, 2021
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

After we have this unnecessary behaviour removed, we can proceed 👍

clients/pkg/logentry/stages/static_labels.go Outdated
}
// If no label source was specified, use the key name
if labelSrc == nil || *labelSrc == "" {
lName := labelName
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this @sankalp-r

dannykopping
dannykopping approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Thanks a lot @sankalp-r! This is a super addition

@dannykopping
Copy link
Contributor

Looks like there are some linter issues @sankalp-r. I'll merge once CI is green

@sankalp-r
add static labels in stages
71dcb75 
Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>
@sankalp-r
Copy link
Contributor Author

Looks like there are some linter issues @sankalp-r. I'll merge once CI is green

Fixed linter issue.
Generated-files check is failing.

@dannykopping
Copy link
Contributor

Looks like there are some linter issues @sankalp-r. I'll merge once CI is green

Fixed linter issue. Generated-files check is failing.

It's a flaky CI job, rerunning

@dannykopping dannykopping merged commit 9e2acd6 into grafana:main Oct 26, 2021
@rfratto rfratto mentioned this pull request Nov 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyriltovena cyriltovena cyriltovena left review comments

@dannykopping dannykopping dannykopping approved these changes

@owen-d owen-d Awaiting requested review from owen-d

@slim-bean slim-bean Awaiting requested review from slim-bean

Assignees

@dannykopping dannykopping

Labels
2.4 size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Attach a static value as custom text to label in pipeline_stages
4 participants
@sankalp-r @CLAassistant @dannykopping @cyriltovena