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

add event.ingested to APM events #3279

Merged
merged 5 commits into from
Feb 5, 2020
Merged

Conversation

graphaelli
Copy link
Member

closes #2934

@codecov-io
Copy link

Codecov Report

Merging #3279 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3279   +/-   ##
=======================================
  Coverage   79.48%   79.48%           
=======================================
  Files         107      107           
  Lines        5607     5607           
=======================================
  Hits         4457     4457           
  Misses       1150     1150

@@ -69,6 +69,11 @@ def test_load_docs_with_template_and_add_transaction(self):
# compare existing ES documents for transactions with new ones
rs = self.es.search(index=index_transaction)
Copy link
Member Author

Choose a reason for hiding this comment

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

looks like we can change these to wait_for_events and consolidate the event.ingested munging

Copy link
Member Author

@graphaelli graphaelli Feb 5, 2020

Choose a reason for hiding this comment

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

@simitt I made some changes after your review to get tests passing - we now disregard the value in event.ingested in all approval tests - would you please peek at b3778dc and ensure you still approve?

@@ -31,6 +31,9 @@
"us": 1494342245999000
},
"@timestamp": "2017-05-09T15:04:05.999Z",
"event": {
"ingested": "2017-05-09T15:04:05.998Z"
Copy link
Contributor

Choose a reason for hiding this comment

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

The timestamps look odd - have you set them once manually? (They should be some date yesterday)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good eye - yes, I set them manually

rs = self.es.search(index=index_transaction)
assert rs['hits']['total']['value'] == 4, "found {} documents".format(rs['count'])
self.approve_docs('transaction', rs['hits']['hits'])
transaction_docs = self.wait_for_events('transaction', 4, index=index_transaction)
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@graphaelli graphaelli merged commit 7558976 into elastic:master Feb 5, 2020
@graphaelli graphaelli deleted the ingest-time branch February 5, 2020 18:11
@jalvz
Copy link
Contributor

jalvz commented Mar 9, 2020

does this need a backport?

@graphaelli
Copy link
Member Author

yes please!

jalvz pushed a commit to jalvz/apm-server that referenced this pull request Mar 11, 2020
jalvz pushed a commit to jalvz/apm-server that referenced this pull request Mar 11, 2020
jalvz pushed a commit to jalvz/apm-server that referenced this pull request Mar 11, 2020
jalvz pushed a commit to jalvz/apm-server that referenced this pull request Mar 11, 2020
jalvz pushed a commit to jalvz/apm-server that referenced this pull request Mar 11, 2020
jalvz pushed a commit to jalvz/apm-server that referenced this pull request Mar 12, 2020
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.

Add ingest timestamp to pipeline
4 participants