Add external labels support #43
Conversation
|
Well, it seems to work xD |
pkg/promtail/client.go
Outdated
| @@ -100,6 +105,11 @@ func (c *Client) run() { | |||
| case <-c.quit: | |||
| return | |||
| case e := <-c.entries: | |||
| // Add any external labels if they exist. | |||
| if len(c.externalLabels) > 0 { | |||
| e.labels = e.labels.Merge(c.externalLabels) | |||
There was a problem hiding this comment.
Here the external labels override the current labels if any overlap. What should be semantics here?
There was a problem hiding this comment.
gouthamve
force-pushed
the
promtail-ext-lbl
branch
from
52611ea to
da6f756
Compare
gouthamve
changed the title
pkg/promtail/client.go
Outdated
| if _, ok := e.labels[ln]; !ok { | ||
| e.labels[ln] = lv | ||
| } | ||
| } |
There was a problem hiding this comment.
You're mutating a map you don't own here. Its probably safe, but better to use the addLabelsMiddleware: https://github.com/grafana/loki/blob/master/pkg/promtail/types.go#L35
There was a problem hiding this comment.
I initially used labels.Merge but that would mean allocating a new map every single time. Not sure we want that?
There was a problem hiding this comment.
Further, the semantics don't match. In Prometheus, if one of the labelsnames in external labels is already present, it is not overwritten...
There was a problem hiding this comment.
There was a problem hiding this comment.
Not sure we want that?
For now performance isn't our main concern, correctness is.
Further, the semantics don't match.
Lets fix that in the middleware then.
gouthamve
force-pushed
the
promtail-ext-lbl
branch
3 times, most recently
from
d881fd0 to
ec009f3
Compare
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
gouthamve
force-pushed
the
promtail-ext-lbl
branch
from
ec009f3 to
f96f2e9
Compare
| @@ -168,6 +173,10 @@ func (c *Client) Stop() { | |||
|
|
|||
| // Handle implement EntryHandler; adds a new line to the next batch; send is async. | |||
| func (c *Client) Handle(ls model.LabelSet, t time.Time, s string) error { | |||
| if len(c.externalLabels) > 0 { | |||
There was a problem hiding this comment.
I did this in the client and did not make it a middleware because it is the client's job to add the external-labels (as we put external labels in the client config). I think it makes sense to keep it here, rather than wrap in the next layer.
Update from upstream repository
Just to trigger build