Skip to content

add transaction.id to errors#437

Merged
graphaelli merged 3 commits intoelastic:masterfrom
graphaelli:error-transaction-id
Dec 28, 2017
Merged

add transaction.id to errors#437
graphaelli merged 3 commits intoelastic:masterfrom
graphaelli:error-transaction-id

Conversation

@graphaelli
Copy link
Copy Markdown
Member

to connect errors to transactions. closes #409

@roncohen
Copy link
Copy Markdown
Contributor

roncohen commented Dec 23, 2017

@graphaelli nice work!
But we should put it in the same place as in with transaction documents. That is, on the root. In other words, we need to move it one level up. That way, the field is the same across all our documents. error.transaction.id -> transaction.id

@graphaelli
Copy link
Copy Markdown
Member Author

@roncohen that makes sense now, pushed up a commit for that. One side-effect is that transaction.id is now a common field - the remaining transaction properties (name, duration, etc) stay valid only on transaction docs.

@graphaelli graphaelli changed the title add error.transaction.id add transaction.id to errors Dec 26, 2017
@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Dec 26, 2017

As the transaction.id is the field for correlation, it's fine that name etc. are only available in the transaction event.

@graphaelli This will need a rebase.

@graphaelli
Copy link
Copy Markdown
Member Author

graphaelli commented Dec 26, 2017

@ruflin I was going to see if merging #443 eliminates the conflict here, do you happen to know?

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Dec 26, 2017

I think you will have to rebase anyways even with #443 as the union is not in this PR yet and Github will not retrigger (I think).

@graphaelli
Copy link
Copy Markdown
Member Author

taking your word for it, rebased & force pushed

}
]
],
"transaction": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this should be inside the elements in errors. Agents can buffer up errors from different transactions

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ugh, my mistake, that makes much more sense.

mapping := []Mapping{
{"errors.context", "context"},
{"transactions.context", "context"},
{"errors.transaction.id", "transaction.id"},
Copy link
Copy Markdown
Member Author

@graphaelli graphaelli Dec 27, 2017

Choose a reason for hiding this comment

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

@roncohen: I wasn't that comfortable with this, any way to do this better?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Making sure i got this correctly: The test checks that everything that get stored as a keyword has the length restriction and vice-versa. We use this mapping to map the json schema names to the final elasticssearch document names. If that's correct, i don't have a better way to do it that what you did here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's how I understand that test as well, thanks for verifying.

Copy link
Copy Markdown
Contributor

@roncohen roncohen left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@graphaelli graphaelli merged commit 41ac107 into elastic:master Dec 28, 2017
@graphaelli graphaelli deleted the error-transaction-id branch December 28, 2017 15:24
simitt pushed a commit to simitt/apm-server that referenced this pull request Jan 8, 2018
* add error.transaction.id

to connect errors to transactions.  closes elastic#409

* move error.transaction.id to root of error

* allow each error in batch to belong to a separate transaction
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.

Connecting errors with transactions

3 participants