output/cloudv2: Flush the aggregated metrics

[codebien]

It implements the drain of the queue of the aggregated metrics and the conversion to Protobuf.

[codecov-commenter]

Codecov Report

Merging #3083 (4f14ff8) into master (ecca789) will decrease coverage by 0.10%.
The diff coverage is 54.86%.

❗ Current head 4f14ff8 differs from pull request most recent head 1d7eed8. Consider uploading reports for the commit 1d7eed8 to get more accurate results

@@            Coverage Diff             @@
##           master    #3083      +/-   ##
==========================================
- Coverage   73.70%   73.61%   -0.10%     
==========================================
  Files         239      240       +1     
  Lines       18258    18364     +106     
==========================================
+ Hits        13457    13518      +61     
- Misses       3933     3974      +41     
- Partials      868      872       +4     

Flag	Coverage Δ
ubuntu	73.61% <54.86%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
output/cloud/expv2/metrics_client.go	58.00% <ø> (ø)
output/cloud/expv2/mapping.go	40.27% <40.27%> (ø)
output/cloud/expv2/output.go	80.45% <70.00%> (-1.25%)	⬇️
output/cloud/expv2/flush.go	83.87% <83.87%> (-16.13%)	⬇️

... and 4 files with indirect coverage changes

[oleiade]

Looks good as far as I can tell 🤝

I've left a couple of nit comments. There are a lot of TODOs left, and I didn't know if we should consider them blocking ❓

[oleiade]

I unfortunately didn't understand this sentence 😢

[codebien]

Extended

[oleiade]

Not being extremely familiar with the plumber of the Go runtime, I had to do a bit of research to understand what was meant by this comment.

Here is an explanation for others who might bump into the same question: the Go memory model does not guarantee the safety of pointers in maps, and the Garbage Collector does not track the usage of pointers as map keys either. It means that the object the pointer points to could be moved or collected by the GC while still being used as a map key, leading to hard-to-diagnose errors. Although I understand that objects are not moved (their address does not change) once they've been allocated in the current version of Go?

[oleiade]

Regardless, if we expect this map to remain small, my first intuition was to switch to a slice or array as commented indeed.

[mstoykov]

@codebien atlas/tagset also depends on this or at least a lot of code using it depend on it.

I can't find any plans on go moving to a moving gc.

There is already a project that just checks that you are using a go version that is known to not have a moving gc - https://github.com/go4org/unsafe-assume-no-moving-gc.

Support for a way to pin memory is already merged, although the use case there is different.

Also, AFAIK using a pointer a map key is not as uncommon and likely will break quite a lot of software, so again - very unlikely to happen. And likely will have a lot of additional problems across the ecosystem.

Maybe open an issue either way?

[codebien]

Removed, not really necessary to hurry about this. The real improvement is mostly to change to the ID or having some slice/array but we need to know better about the tracking/untracking.

[mstoykov]

@codebien atlas/tagset also depends on this or at least a lot of code using it depend on it.

I can't find any plans on go moving to a moving gc.

There is already a project that just checks that you are using a go version that is known to not have a moving gc - https://github.com/go4org/unsafe-assume-no-moving-gc.

Support for a way to pin memory is already merged, although the use case there is different.

Also, AFAIK using a pointer a map key is not as uncommon and likely will break quite a lot of software, so again - very unlikely to happen. And likely will have a lot of additional problems across the ecosystem.

Maybe open an issue either way?

[mstoykov]

LGTM!

The gauge comment is the most blockign, but even that isn't really blocking, just checking why are we sending so many stuff for gauge.

[mstoykov]

LGTM! Left a not blocking comment.