Skip to content
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

e2e: add test for cpnp replication #7

Merged

Conversation

GiedriusS
Copy link

Add e2e test that shows that cpnp replication works. Remove some unused code.

@GiedriusS GiedriusS force-pushed the add_e2e_test_capnp branch 2 times, most recently from 05cb1fc to c6145fb Compare October 16, 2024 12:56
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS
Copy link
Author

Very weird - writes succeed and I can see the timestamps/values coming in as expected, and the series is in the WAL but I can't query them :/ something is wrong with the writer but I cannot spot anything wrong so far.

@fpetkovski
Copy link
Owner

I added some logging to the writer and I can also see

Appending sample {__name__="myself"} 1729090293939 1
err: <nil>

Maybe something with the timestamp format?

@fpetkovski
Copy link
Owner

Also seeing metrics in the ingestor

# HELP prometheus_tsdb_head_samples_appended_total Total number of appended samples.
# TYPE prometheus_tsdb_head_samples_appended_total counter
prometheus_tsdb_head_samples_appended_total{tenant="default-tenant",type="float"} 1
prometheus_tsdb_head_samples_appended_total{tenant="default-tenant",type="histogram"} 0

@fpetkovski
Copy link
Owner

fpetkovski commented Oct 16, 2024

@GiedriusS changing this line from unsafe.String(&b[0], len(b)) to string(b) makes the test pass. I assume that labels.Copy here only does a shallow copy of labels and actual name and value bytes get lost at some point.

For now we can either remove the unsafe conversion, or we can have a deep copy of labels which re-allocates the name and value strings.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS
Copy link
Author

Added a strings.Clone() where it is really necessary. I think it is preferable to always making a copy because creating a new series is a much rarer occurrence than adding new samples to an existing series.

// NOTE(GiedriusS): do a deep copy because the labels are reused in the capnp message.
// Creation of new series is much rarer compared to adding extra samples
// to an existing series.
for i := range lset {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to use lset.Range

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this on my branch.

@fpetkovski fpetkovski merged commit 14c840f into fpetkovski:capnproto-replication Oct 17, 2024
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants