Should we potentially use the RLock() here and then only lock if we need to overwrite s.meta? Or potentially introduce a new metadataLock to memSeries?

I'm not really sure which is the best option, but personally I'd avoid adding extra fields to an already-large struct for each and every series.

Were you also thinking of something like 045ec58?

Hmmm, those locks are a bit scary here. I assume lock is here because we want the latest metadata across all sources of a single metric name, right? Which could be inconsistent during scrape. Or am I wrong?

Nevertheless, we are on the hot path of scraping which has to be efficient, so we have to ideally benchmark this path. WDYT? Ideally with cases of potential lock congestion. If there is regression, we could opt in for some less consistent framework for choosing what metadata should stay.

Also for "error?" cases where process A is registering __name__=metric1 with help ABC and process B registering __name__=metric1 with help ABC2, we will constantly update those metadata back and worth in worse case, right?

Sorry @bwplotka for the super late reply.

Yes, I think we can go with a feature flag until we see its effects on larger-scale Prometheus deployments.

I assume lock is here because we want the latest metadata across all sources of a single metric name, right? Which could be inconsistent during scrape. Or am I wrong?

Yes, that's what we hold as metadata in the scrapeCache. As you said, this inconsistency will happen when two different scrape_configs share a common metric name and try to simultaneously update its metadata, right? I think that yes, in the worst case that two processes register different metadata in each scrape iteration, we will be recording all these updates.

so we have to ideally benchmark this path. WDYT? Ideally with cases of potential lock congestion

I'm building a benchmark to measure the effect and will come back to you. We could also consider using read locks as we're checking whether an update is needed.

Hey @bwplotka! About the metaMtx mutex in scrapeCache; if I'm getting this right, the scrapeCache is unique per scrapeLoop.

In that case, there will be no contention for this mutex between multiple scrapers, right? Each scraper will have its own lock to use as if it's encountering new metadata. The things that do run concurrently and might contend the mutex are a) the methods used by the MetadataWatcher in the QueueManager and b) the MetadataMetricsCollector which reports metrics to the scrape manager. If believe those use cases weren't needed, we wouldn't even need metaMtx.

If that's true, does it relieve some of the concerns around these locks?

I've run some benchmarks (tpaschalis@177d95a) to get a feel of the effect of this on the scrape loop after rebasing with the current main (070e409).

The first benchmark is the existing BenchmarkScrapeLoopAppend. The second one changes metadata on every scrape iteration, while the third one both changes metadata on every iteration and tries to obtain the metadata mutex every millisecond (not a realistic scenario with the current use cases).

$ benchstat main pr10312
name                                               old time/op  new time/op  delta
ScrapeLoopAppend-8                                 41.9µs ± 3%  43.4µs ± 6%  +3.50%  (p=0.000 n=19+20)
ScrapeLoopAppend_MetadataChangesAlways-8           47.6µs ± 2%  51.7µs ± 2%  +8.71%  (p=0.000 n=19+20)
ScrapeLoopAppend_MetadataChangesAlwaysContended-8  48.9µs ± 3%  52.7µs ± 1%  +7.88%  (p=0.000 n=19+17)

How do you feel about these benchmarks?

if I'm getting this right, the scrapeCache is unique per scrapeLoop. this is how I read the code as well. The newLoop function, part of scrapePool, takes in the scrapeLoopOptions struct. If the cache field on that struct is nil a new scrape cache is created. When a sync happens and the new scrape loops are started (some of which won't really have changed) the existing scrape loops are passed where/when relevant.

The implementation feels like we will keep all the historical metadata changes of a series as long as the series exists. Should we not keep just the last metadata of a series in this case?

Yeah, that's true. I thought this wouldn't have much impact as a) metadata isn't usually rapidly changing, and b) if the metadata does change then it might make sense to be able to correlate it with the values associated with a series (eg. the unit changing from seconds to milliseconds).

If we do want to keep the latest metadata entry only, we don't have any timestamps associated with them so I can't say how we'd go about it off the top of my head, but I'll take a look just in case we want to move to that direction.

It makes sense to keep a metadata around until all the samples after that have been removed from the checkpoint until the new metadata appears for the reason you said - make sense of the old data. But I am not sure how efficiently we can do that. Once idea is to store the timestamp of the last sample seen when the metadata was updated.

If we do want to keep the latest metadata entry only, we don't have any timestamps associated with them so I can't say how we'd go about it off the top of my head, but I'll take a look just in case we want to move to that direction.

This one is relatively simple but with one complexity. Similar to the keep(ref) method that we have, we have another keepMeta(ref, meta), which returns true if the metadata matches the one present in the head block currently, i.e. the latest. Else false. Problem here is that, between 2 checkpoints, a series might have metadata M1, then switched to M2, then again back to M1, in which case we will end up keeping 2 copies of M1. We need to think how to dedupe this. Maybe a timestamp of some sort will help to identify the latest.

The WAL is written to in-order, right? We can assume that for a given ref ID, metadata entries are written in-order and we can keep only the latest metadata entry for each ref ID. We shouldn't need to include timestamps, IIUC.

Sorry for the late response. I think @rfratto's suggestion should work nicely.

One way to go about this would be to just keep track of the latest metadata for each series in the for r.Next() loop, and then flush them separately along the remaining records in the end. I'll try it out to see how it works in practice, but I also wanted to hear how it sounds to y'all.

Edit: I've implemented the deduplication logic on 36cb2b6 along with a relevant test. Let me know how it looks to you, I'll try to see if I can bring some benchmarks.

With this, I think we might be able to avoid complicating the WAL storage format.

If we do not care of old metadata, then definitely, just keep the last one!

One of the use-cases brought up was keeping track of changing metadata. Do we not care about knowing what the series meant a month ago and only care about what it means now?

LGTM assuming these are the current metric types and how/where we want to encode them for source of truth. @codesome ?

Could you provide some more context on this please? I'm not sure what you meant 😕

I just happen to know that these are the types present in the client_golang. (Edit: also the ones recognized by the scrape)

I am a little unsure about doing it this way. Since there is a few % extra memory usage in general with metadata, holding this map will increase the peak memory requied a bit more.

What I would suggest is having another keep method, let's call it keepMetadata(ref, metadata). Like how keep() checks if the series exists, keepMetadata(..) will check if the series exists, and if it does, it will check if the metadata matches the one present in the memory. That way we don't have to keep this map around.

A note for future (no action required right now): Does this have any noticeable memory impact when feature is disabled? Should it be a pointer instead?

By my quick count this struct was previously 22 8-byte words, and adding meta would take it to 28 (+ 3 strings).
For 10 million series this adds 60MB, typically 120MB in memory usage before GC.

(Not counting the contents of the strings, which I hope are shared across all series with the same values)

which I hope are shared across all series with the same values

@tpaschalis is this the case? The only thing that I see can be share is the type and unit

Sorry I went from your question "when feature is disabled" to the values, which obviously only exist when the feature is enabled. However it's valid to ask the memory impact in that case too.

No, the UNIT and HELP strings are not currently shared between series.

Should we look into interning them? We have a PR for being able to share the interner used by remote_write with the scrape loop, but admittedly it has made little progress.

(of course, any comments to the other PR or ideas here are more than welcome 😄 )

Just to clarify: where the same metric appears many times in a single scrape (example), the help string will only be parsed once; are you sure that value isn't shared?
Interning will help further, sharing across different scrapes, but has some concurrency implications.

Oh I see what you mean. We aren't changing anything about the way the HELP or UNIT strings are parsed; they are indeed shared between all series with the same metric name, but not across scrapes yet.

Feels weird to me that we do a text->number conversion here inside tsdb.
Why not do it at scrape time where we do all the other text parsing?

Thanks for bringing this up. This is relevant to your next comment as well.

The enum-like MetricType was added as an encoding/decoding-specific optimisation after this suggestion and it didn't occur to me to make this conversion from the scrape loop.

One drawback I see is that the textparse.MetricType is shared between many structs, so there would a bigger change to review around the scrape loop.
For example, the current MetricType implementation of how metadata arrives to the remote_write would either need an extra conversion, or it would need to propagate the enum-type correctly, resulting in even more changes.

Why is this string-like rather than enum-like?
I can see it was a string previously, but now you are introducing an enum it seems unnatural to have to convert it back to a string to populate this field.

Tied to the previous comment; mainly for backwards-compatibility and not having to touch more files in an already-big PR.

I can produce a commit with how using the enum here would look and get back to you for a second round.