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.

Sign up for GitHub

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

Jump to bottom

Fix timestamp handling in remote_write #21166

Merged
merged 4 commits into from
Sep 23, 2020
Merged

Fix timestamp handling in remote_write #21166

merged 4 commits into from
Sep 23, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Sep 18, 2020 •
edited
Loading

What does this PR do?

Fixes how we handle timestamps when we process Prometheus Samples using remote_write. Before this change it could be possible that we miss some metrics in case of having the same metric in the same received chunk (with different timestamps). Metrics with different timestamps should go in different final events and hence timestamp should be taken into account when grouping metrics into events.

Why is it important?

To avoid losing data due to wrong grouping.

Checklist

  • My code follows the style guidelines of this project
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@ChrsMark
Fix timestamp handling in remote_write
e4bbfc7 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added bug needs_backport PR is waiting to be backported to other branches. Team:Platforms Label for the Integrations - Platforms team v7.10.0 v7.9.2 labels Sep 18, 2020
@ChrsMark ChrsMark self-assigned this Sep 18, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Sep 18, 2020
@ChrsMark
Add changelog and test
ac09cad 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
exekias
exekias reviewed Sep 18, 2020
View reviewed changes
metricbeat/module/prometheus/remote_write/data.go Outdated
Comment on lines 60 to 63
// join metrics with same labels and same timestamp in a single event
timeStampedLabels := labels.Clone()
timeStampedLabels.Put("timestamp", metric.Timestamp.Time())
labelsHash := timeStampedLabels.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would try to avoid cloning to save memory and cycles. How about appending the timestamp to the labelsHash?

Something like sampleHash := labels.String() + metric.Timestamp.Time()

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

exekias
exekias suggested changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you for taking on this! did you happen to reproduce the issue?

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 18, 2020 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #21166 updated]

  • Start Time: 2020-09-21T08:34:48.134+0000

  • Duration: 61 min 26 sec

Test stats 🧪

Test Results
Failed 0
Passed 3050
Skipped 675
Total 3725

@ChrsMark
Copy link
Member Author

Thank you for taking on this! did you happen to reproduce the issue?

Not with a real Prometheus setup but the testcase https://github.com/elastic/beats/pull/21166/files#diff-08a773efe40f622118ed7a6596ea3adaR45 reproduces exactly a scenario where a metric sent twice in a chunk could be saved only once from us due to the bad timestamp handling.

@ChrsMark ChrsMark added [zube]: In Progress in progress Pull request is currently in progress. labels Sep 23, 2020
@ChrsMark ChrsMark merged commit d2a8922 into elastic:master Sep 23, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Sep 23, 2020
@ChrsMark
Fix timestamp handling in remote_write (elastic#21166)
2310392 
(cherry picked from commit d2a8922)
@ChrsMark ChrsMark mentioned this pull request Sep 23, 2020
Merged
3 tasks
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Sep 23, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Sep 23, 2020
@ChrsMark
Fix timestamp handling in remote_write (elastic#21166)
f91fadb 
(cherry picked from commit d2a8922)
@ChrsMark ChrsMark mentioned this pull request Sep 23, 2020
Merged
3 tasks
ChrsMark added a commit that referenced this pull request Sep 23, 2020
@ChrsMark
Fix timestamp handling in remote_write (#21166) (#21236)
95c4430 
(cherry picked from commit d2a8922)
ChrsMark added a commit that referenced this pull request Sep 23, 2020
@ChrsMark
Cherry-pick #21166 to 7.9: Fix timestamp handling in remote_write (#2…
4eae9b5 
…1237)
v1v added a commit to v1v/beats that referenced this pull request Sep 24, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/ci-pipeli…
c59bffe 
…ne-2.0-arm

* upstream/master: (29 commits)
  Fix librpm installation in auditbeat build (elastic#21239)
  Fix prometheus default config (elastic#21253)
  Fix dev guide test command (elastic#21254)
  Move aws lambda metricset to GA (elastic#21255)
  [Docs] Typo in table syntax (elastic#20227)
  [ECS] Adds related.hosts to capture all hostnames and host identifiers on an event. (elastic#21160)
  Add recursive split to httpjson (elastic#21214)
  [DOCS] Add beat specific start widgets (elastic#21217)
  Fix timestamp handling in remote_write (elastic#21166)
  Fix aws, azure and googlecloud compute dashboards (elastic#21098)
  Add acceptable event log keys to winlog (elastic#21205)
  Add elastic-agent to gitignore (elastic#21219)
  Add cloudfoundry tags to events (elastic#21177)
  [Ingest Manager] Agent includes pgp file (elastic#19480)
  Add compatibility note about ingress-controller-v0.34.1 (elastic#21209)
  [Ingest Manager] Support for UPGRADE_ACTION (elastic#21002)
  Fix libbeat.output.*.bytes metrics of Elasticsearch output (elastic#21197)
  [packaging] use docker.elastic.co/ubi8/ubi-minimal (elastic#21154)
  Add host inventory metrics to system module (elastic#20415)
  [Filebeat][Azure Module] Fixing event.outcome from result_type issue (elastic#20998)
  ...
v1v added a commit to v1v/beats that referenced this pull request Sep 24, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/ci-pipeli…
778c037 
…ne-2.0

* upstream/master: (33 commits)
  Stop running agent container as root by default (elastic#21213)
  Stop running auditbeat container as root by default (elastic#21202)
  Fix autodiscover flaky tests (elastic#21242)
  [Ingest Manager] Enabled dev builds (elastic#21241)
  Fix librpm installation in auditbeat build (elastic#21239)
  Fix prometheus default config (elastic#21253)
  Fix dev guide test command (elastic#21254)
  Move aws lambda metricset to GA (elastic#21255)
  [Docs] Typo in table syntax (elastic#20227)
  [ECS] Adds related.hosts to capture all hostnames and host identifiers on an event. (elastic#21160)
  Add recursive split to httpjson (elastic#21214)
  [DOCS] Add beat specific start widgets (elastic#21217)
  Fix timestamp handling in remote_write (elastic#21166)
  Fix aws, azure and googlecloud compute dashboards (elastic#21098)
  Add acceptable event log keys to winlog (elastic#21205)
  Add elastic-agent to gitignore (elastic#21219)
  Add cloudfoundry tags to events (elastic#21177)
  [Ingest Manager] Agent includes pgp file (elastic#19480)
  Add compatibility note about ingress-controller-v0.34.1 (elastic#21209)
  [Ingest Manager] Support for UPGRADE_ACTION (elastic#21002)
  ...
v1v added a commit to v1v/beats that referenced this pull request Sep 28, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/windows-2008
2cbe6d9 
* upstream/master: (417 commits)
  libbeat/cmd/instance: report cgroup stats (elastic#21113)
  Configurable index template loading (elastic#21212)
  [Ingest Manager] Thread safe sorted set (elastic#21290)
  Change mirror of kafka download (elastic#19645)
  [Ingest manager] Copy Action store on upgrade (elastic#21298)
  [CI] Pipeline 2.0 for monorepos (elastic#20104)
  Stop running agent container as root by default (elastic#21213)
  Stop running auditbeat container as root by default (elastic#21202)
  Fix autodiscover flaky tests (elastic#21242)
  [Ingest Manager] Enabled dev builds (elastic#21241)
  Fix librpm installation in auditbeat build (elastic#21239)
  Fix prometheus default config (elastic#21253)
  Fix dev guide test command (elastic#21254)
  Move aws lambda metricset to GA (elastic#21255)
  [Docs] Typo in table syntax (elastic#20227)
  [ECS] Adds related.hosts to capture all hostnames and host identifiers on an event. (elastic#21160)
  Add recursive split to httpjson (elastic#21214)
  [DOCS] Add beat specific start widgets (elastic#21217)
  Fix timestamp handling in remote_write (elastic#21166)
  Fix aws, azure and googlecloud compute dashboards (elastic#21098)
  ...
v1v added a commit to v1v/beats that referenced this pull request Sep 28, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/windows-7-32
a823b84 
* upstream/master: (399 commits)
  libbeat/cmd/instance: report cgroup stats (elastic#21113)
  Configurable index template loading (elastic#21212)
  [Ingest Manager] Thread safe sorted set (elastic#21290)
  Change mirror of kafka download (elastic#19645)
  [Ingest manager] Copy Action store on upgrade (elastic#21298)
  [CI] Pipeline 2.0 for monorepos (elastic#20104)
  Stop running agent container as root by default (elastic#21213)
  Stop running auditbeat container as root by default (elastic#21202)
  Fix autodiscover flaky tests (elastic#21242)
  [Ingest Manager] Enabled dev builds (elastic#21241)
  Fix librpm installation in auditbeat build (elastic#21239)
  Fix prometheus default config (elastic#21253)
  Fix dev guide test command (elastic#21254)
  Move aws lambda metricset to GA (elastic#21255)
  [Docs] Typo in table syntax (elastic#20227)
  [ECS] Adds related.hosts to capture all hostnames and host identifiers on an event. (elastic#21160)
  Add recursive split to httpjson (elastic#21214)
  [DOCS] Add beat specific start widgets (elastic#21217)
  Fix timestamp handling in remote_write (elastic#21166)
  Fix aws, azure and googlecloud compute dashboards (elastic#21098)
  ...
v1v added a commit to v1v/beats that referenced this pull request Sep 28, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/windows-10
a8cb7d8 
* upstream/master: (60 commits)
  libbeat/cmd/instance: report cgroup stats (elastic#21113)
  Configurable index template loading (elastic#21212)
  [Ingest Manager] Thread safe sorted set (elastic#21290)
  Change mirror of kafka download (elastic#19645)
  [Ingest manager] Copy Action store on upgrade (elastic#21298)
  [CI] Pipeline 2.0 for monorepos (elastic#20104)
  Stop running agent container as root by default (elastic#21213)
  Stop running auditbeat container as root by default (elastic#21202)
  Fix autodiscover flaky tests (elastic#21242)
  [Ingest Manager] Enabled dev builds (elastic#21241)
  Fix librpm installation in auditbeat build (elastic#21239)
  Fix prometheus default config (elastic#21253)
  Fix dev guide test command (elastic#21254)
  Move aws lambda metricset to GA (elastic#21255)
  [Docs] Typo in table syntax (elastic#20227)
  [ECS] Adds related.hosts to capture all hostnames and host identifiers on an event. (elastic#21160)
  Add recursive split to httpjson (elastic#21214)
  [DOCS] Add beat specific start widgets (elastic#21217)
  Fix timestamp handling in remote_write (elastic#21166)
  Fix aws, azure and googlecloud compute dashboards (elastic#21098)
  ...
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@ChrsMark
Cherry-pick elastic#21166 to 7.9: Fix timestamp handling in remote_wr…
d072f43 
…ite (elastic#21237)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exekias exekias exekias approved these changes

@jsoriano jsoriano Awaiting requested review from jsoriano

Assignees

@ChrsMark ChrsMark

Labels
bug in progress Pull request is currently in progress. Team:Platforms Label for the Integrations - Platforms team v7.9.2 v7.10.0 [zube]: In Progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChrsMark @elasticmachine @exekias