Move GlobalRefMap into service instead of global var. #5489

mattdurham · 2023-10-16T14:09:56Z

PR Description

This is mostly a shift from global var to service with ideally no change in behavior.

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
Config converters updated

…f_service

mattdurham · 2023-10-20T14:26:56Z

ugh fixing some things when i blindly hit update branch from github ui

ptodev

I'm surprised that something like this is global. Isn't there going to be interference between components in certain edge cases? For example, if we have two different scrape -> remote_write pipelines, where there we scrape different endpoints but the metric names and labels are the same? In that case, for example staleness marker won't be applied properly if one series goes stale but the other isn't, and both are series with the same hash but actually come from different endpoints?

ptodev · 2023-10-23T11:24:29Z

component/otelcol/exporter/prometheus/prometheus.go

+	if err != nil {
+		return nil, err
+	}
+	ls := service.(labelstore.LabelStore)


Is this label store still valid even if the config is reloaded? Should we use a new label store instead, and update it in the component's Update function?

Services require that items returned from service.data are consistent throughout the lifetime of the process.

I wonder if there are situations when this would fail. For example, this scenario might be an issue:

An Agent does scrape -> remote_write.

The config is reloaded to scrape a different endpoint, but this other endpoint has the same labels and metrics (so the series look the same).

At that point, the map would get confused and think that it's still tracking the old series.

I'm not sure if this has any adverse effects, but seems like a weird side-effect. There might be similar side-effects if we reload the config and point it to a different remote_write endpoint too.

That would be fine since in the current iteration they are the same to it and should be. At the heart its a int to string mapping and a int to local remote write int mapping.

That would be fine since in the current iteration they are the same to it and should be.

I don't follow. Why using a stale LocalRefID, from the previous configuration of a remote_write is not a problem?

In the above scenario its the scraper that is changing not the remote_write. In the case of remote_write changing, I don't see anywhere we reset the walStorage in remote_write component on the updating so the local id cache should stay the same.

ptodev · 2023-10-23T11:41:02Z

service/labelstore/service.go

+	}
+
+	labelHash := lbls.Hash()
+	globalID, found := s.labelsHashToGlobal[labelHash]


Does this handle hash collisions?

It does not but its what we have been running for a year or so.

mattdurham · 2023-10-23T14:01:25Z

There should be no change to any underlying behavior. Beforehand the globalrefmap was a global var but instead moving it into a service. Its possible in the above scenario with two scrape loops for there to be overlap, though its not an issue today because we haven't wired in staleness markers. Its possible it would be an issue when we do wire them in.

pkg/flow/componenttest/componenttest.go

thampiotr · 2023-10-24T12:55:49Z

service/labelstore/data.go

+type LabelStore interface {
+	GetOrAddLink(componentID string, localRefID uint64, lbls labels.Labels) uint64
+	GetOrAddGlobalRefID(l labels.Labels) uint64
+	GetGlobalRefID(componentID string, localRefID uint64) uint64
+	GetLocalRefID(componentID string, globalRefID uint64) uint64
+	AddStaleMarker(globalRefID uint64, l labels.Labels)
+	RemoveStaleMarker(globalRefID uint64)
+	CheckStaleMarkers()
+}


Could we add some comments on this interface for clarity what this is for and when to use the methods? I think some implementation methods already have comments that could be a starting point.

Sounds good will do.

service/labelstore/service.go

thampiotr · 2023-10-24T13:05:21Z

service/labelstore/service.go

+}
+
+// staleDuration determines how often we should wait after a stale value is received to GC that value
+var staleDuration = time.Minute * 10


Should we take this opportunity and make this configurable?

Since it the code path is unused and trying to avoid changing behavior would like to push this to another pr.

thampiotr · 2023-10-24T13:07:48Z

service/labelstore/service.go

+var staleDuration = time.Minute * 10
+
+// CheckStaleMarkers is called to garbage collect and items that have grown stale over stale duration (10m)
+func (s *service) CheckStaleMarkers() {


CheckStaleMarkers doesn't sound like a good name IMO - doesn't reflect that this will delete old markers and garbage-collect effectively. We can call it just GarbageCollection() as it may serve multiple purposes.

How do you feel about CheckAndRemoveStaleMarkers, I feel GarbageCollection is a bit to general at the moment.

Sounds good. Or shorter CleanUpStaleMarkers.

thampiotr · 2023-10-24T13:08:43Z

service/labelstore/service.go

+var staleDuration = time.Minute * 10
+
+// CheckStaleMarkers is called to garbage collect and items that have grown stale over stale duration (10m)
+func (s *service) CheckStaleMarkers() {


This seems to only be called from a test. Shouldn't we have something calling this?

We should but avoiding it for this PR since this is fork lifting functionality with ideally zero change.

thampiotr · 2023-10-24T13:19:17Z

component/otelcol/exporter/prometheus/prometheus.go

+	if err != nil {
+		return nil, err
+	}
+	ls := service.(labelstore.LabelStore)


That would be fine since in the current iteration they are the same to it and should be.

I don't follow. Why using a stale LocalRefID, from the previous configuration of a remote_write is not a problem?

thampiotr · 2023-10-24T13:20:10Z

component/prometheus/remotewrite/remote_write.go

@@ -103,10 +112,10 @@ func New(o component.Options, c Arguments) (*Component, error) {
 				return 0, fmt.Errorf("%s has exited", o.ID)
 			}

-			localID := prometheus.GlobalRefMapping.GetLocalRefID(res.opts.ID, uint64(globalRef))
+			localID := ls.GetLocalRefID(res.opts.ID, uint64(globalRef))


This localID may be for a previous configuration of the component and may no longer be valid. Is that okay?

There might be an issue with queue manager and seriesLabels but I think to fix that would require some Prometheus changes. Worst case a series would drop until the wal watcher reread a segment with them. This would also be a bug in our current incarnation.

True, that would already be a bug :) Maybe we could prevent it by adding a component 'incarnation' ID to the current component ID, so they don't reuse labels when config changes. Let's park this discussion for the future.

Co-authored-by: Piotr <[email protected]>

thampiotr

LGTM!

mattdurham · 2023-10-24T14:47:26Z

FYI created #5585 to handle wiring up garbage collection.

…balref_service

mattdurham added 4 commits September 28, 2023 10:10

commit changes

012bc8e

Initial commit for converting global ref id to a service.

14feba0

merge from main

fb9a1db

Merge branch 'main' into globalref_service

f670463

mattdurham changed the title ~~WIP: Move GlobalRefMap into service instead of global var.~~ Move GlobalRefMap into service instead of global var. Oct 16, 2023

mattdurham marked this pull request as ready for review October 16, 2023 14:17

mattdurham requested review from a team as code owners October 16, 2023 14:17

mattdurham added 5 commits October 16, 2023 10:31

Fix lint

9856866

Merge remote-tracking branch 'origin/globalref_service' into globalre…

f33d239

…f_service

Fix new tests.

b2583ec

fix lint

6028c69

Merge branch 'main' into globalref_service

c67e48d

mattdurham requested review from thampiotr, tpaschalis and erikbaranowski October 18, 2023 13:40

Merge branch 'main' into globalref_service

0859bbd

mattdurham added 2 commits October 20, 2023 10:52

make globalrefmap a service

4d458f4

Merge branch 'main' into globalref_service

8a81203

ptodev reviewed Oct 23, 2023

View reviewed changes

Merge branch 'main' into globalref_service

f8bff2e

thampiotr reviewed Oct 24, 2023

View reviewed changes

Update service/labelstore/service.go

4622849

Co-authored-by: Piotr <[email protected]>

thampiotr approved these changes Oct 24, 2023

View reviewed changes

mattdurham added 2 commits October 24, 2023 10:48

pr feedback

5ba8655

Merge branch 'globalref_service' of github.com:grafana/agent into glo…

479d2e6

…balref_service

Merge branch 'main' into globalref_service

189ce23

mattdurham enabled auto-merge (squash) October 24, 2023 14:50

mattdurham disabled auto-merge October 24, 2023 14:51

mattdurham merged commit b4b267d into main Oct 24, 2023

mattdurham deleted the globalref_service branch October 24, 2023 15:12

github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move GlobalRefMap into service instead of global var. #5489

Move GlobalRefMap into service instead of global var. #5489

mattdurham commented Oct 16, 2023

mattdurham commented Oct 20, 2023

ptodev left a comment

ptodev Oct 23, 2023

mattdurham Oct 23, 2023

ptodev Oct 23, 2023

mattdurham Oct 23, 2023

thampiotr Oct 24, 2023

mattdurham Oct 24, 2023

ptodev Oct 23, 2023

mattdurham Oct 23, 2023

mattdurham commented Oct 23, 2023

thampiotr Oct 24, 2023

mattdurham Oct 24, 2023

thampiotr Oct 24, 2023

mattdurham Oct 24, 2023

thampiotr Oct 24, 2023

mattdurham Oct 24, 2023

thampiotr Oct 24, 2023

thampiotr Oct 24, 2023

mattdurham Oct 24, 2023

thampiotr Oct 24, 2023

thampiotr Oct 24, 2023

mattdurham Oct 24, 2023

thampiotr Oct 24, 2023

thampiotr left a comment

mattdurham commented Oct 24, 2023

Move GlobalRefMap into service instead of global var. #5489

Move GlobalRefMap into service instead of global var. #5489

Conversation

mattdurham commented Oct 16, 2023

PR Description

PR Checklist

mattdurham commented Oct 20, 2023

ptodev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdurham commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thampiotr left a comment

Choose a reason for hiding this comment

mattdurham commented Oct 24, 2023