-
Notifications
You must be signed in to change notification settings - Fork 487
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
feat(loki/src/k8s): configurable default instance
label for scraped pod logs
#6739
feat(loki/src/k8s): configurable default instance
label for scraped pod logs
#6739
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc input/comments
Over to @grafana/grafana-agent-maintainers for a code review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved from a docs standpoint
|
||
Name | Type | Description | Default | Required | ||
------------------------ | ------------------- | ------------------------------------------------------------- | ------- | -------- | ||
`enabled` | `bool` | Enable setting default instance label value. | true | no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a separate enabled value? Would having a value inherently make it true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would an empty string indicate of no default instance
label for every podlogs or having an empty default instance
label?
I'm not sure if having a separate value is the way to go here but I feel like its explicitness would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK in both Prometheus and Loki, an empty value label is considered equivalent to a label that doesn't exist.
So in this case, you wouldn't really be able to have an empty instance label. I'd agree this makes sense as an attribute instead of a block. It should only override defaultInstance
when it's expliticly supplied as a non-empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool knowing the practice put in place by other parts of the ecosystem :D
I'd try implementing it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that the original requester would like to disable the behavior of setting default instance
label too 🤦
And having an empty string to disable the behavior would be breaking change, unless we choose a token to signify the act of disabling, like defaultInstanceLabel="disabled"
or something, which doesn't seem right to me 🤔
To have both the ability to disable the behavior and to set a custom default instance
label, I think we gotta resort to block now (unless having a separate attribute :D)
… pod logs Signed-off-by: hainenber <[email protected]>
… actual value Signed-off-by: hainenber <[email protected]>
3281751
to
375f654
Compare
This PR has not had any activity in the past 30 days, so the |
This PR has not had any activity in the past 30 days, so the |
Closing as the linked issue is closed too. |
PR Description
Which issue(s) this PR fixes
Fixes grafana/alloy#211
Notes to the Reviewer
PR Checklist
Config converters updated