Mobile transaction type #184
Conversation
LikeTheSalad
requested review from
gregkalapos and
hegerchr
and removed request for
a team
QuestionThese changes set |
gregkalapos
left a comment
gregkalapos
left a comment
There was a problem hiding this comment.
lgtm. Thanks for the PR! 🎉
These changes set
transaction.typetomobilebased on the elastic-specific type attribute. Should we remove thetypeattribute from the span after we use it to settransaction.type?
I'd keep it. In this lib, we typically don't remove any attributes. We could save some storage if we remove it, on the other hand, it'd be very confusing if we start removing fields from incoming data. So my take is to keep it.
| s.userAgentName = v.Str() | ||
| case semconv27.AttributeUserAgentVersion: | ||
| s.userAgentVersion = v.Str() | ||
| case "type": |
There was a problem hiding this comment.
How will this attribute be set?
There was a problem hiding this comment.
It's set by the EDOT mobile SDKs specified here.
There was a problem hiding this comment.
Maybe also add elasticattr.TransactionType do this case so that if transaction.type is set already, it's not overridden?
There was a problem hiding this comment.
Maybe also add
elasticattr.TransactionTypedo this case so that iftransaction.typeis set already, it's not overridden?
I'm ok with validating that transaction.type won't change if it's already present. It's a different use case though it seems like it'd be easy enough to add it in this PR. What do you think, @gregkalapos @lahsivjar ?
There was a problem hiding this comment.
Maybe also add elasticattr.TransactionType do this case so that if transaction.type is set already, it's not overridden?
I think overriding behaviour should be handled for other attributes too, but I am not sure if not overriding in all cases would be the correct option. I have created #185 to discuss it further.
There was a problem hiding this comment.
Ok by me to include it in this PR. update: @lahsivjar was faster - let's discuss in #185.
There was a problem hiding this comment.
I think overriding behaviour should be handled for other attributes too, but I am not sure if not overriding in all cases would be the correct option. I have created #185 to discuss it further.
Got it. Thanks, @lahsivjar. I won't add that change here then.
There was a problem hiding this comment.
I think we should do it in a separate PR since this concern might apply to other attributes too, but I don't have a strong opinion here. Does that makes sense @gregkalapos @felixbarny ? Or do we think only transaction.type needs to be handled this way?
NVM, let's discuss in #185
| } | ||
|
|
||
| // Tests the enrichment logic for elastic's transaction definition with type. | ||
| func TestElasticTransactionEnrichWithType(t *testing.T) { |
There was a problem hiding this comment.
nit: Do we need a new test? The contents feels like they will fit as a subtest for TestElasticTransactionEnrich
There was a problem hiding this comment.
Got it. I've just moved the test cases to existing tests.
Co-authored-by: Vishal Raj <vishal.raj@elastic.co>
Co-authored-by: Vishal Raj <vishal.raj@elastic.co>
lahsivjar
left a comment
lahsivjar
left a comment
There was a problem hiding this comment.
Missed a couple of places to remove hasValue variable. Rest LGTM!
| s.userAgentName = v.Str() | ||
| case semconv27.AttributeUserAgentVersion: | ||
| s.userAgentVersion = v.Str() | ||
| case "type": |
There was a problem hiding this comment.
Maybe also add elasticattr.TransactionType do this case so that if transaction.type is set already, it's not overridden?
I think overriding behaviour should be handled for other attributes too, but I am not sure if not overriding in all cases would be the correct option. I have created #185 to discuss it further.
4 participants
Fixes elastic/kibana#216980
These changes aim to set
transaction.type=mobileto all root spans coming from our mobile agents.