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

Server accepts and records empty transaction type #3141

Closed
axw opened this issue Jan 11, 2020 · 5 comments
Closed

Server accepts and records empty transaction type #3141

axw opened this issue Jan 11, 2020 · 5 comments

Comments

@axw
Copy link
Member

axw commented Jan 11, 2020

A user of the Go agent found an issue by sending a transaction with an empty type, like this:

tx := apm.DefaultTracer.StartTransaction("name", "")
tx.End()

This led to the APM app in Kibana showing an empty Transactions page for the service. I think we should either reject such transactions at the server, or else set a default transaction type.

I also think the agents should also not allow transaction types to be empty. Some agents set it to "custom" if not specified.

@simitt
Copy link
Contributor

simitt commented Jan 14, 2020

Rejecting data would force customers to upgrade to an agent version where the transaction type will never be unset. If agents solve this in a way to immediately report to the users that the transaction type needs to be set, this might be an option. If agents simply set the custom type for such cases I think it would be better the APM Server accepts such requests and sets custom itself, as it is less user pain with the same outcome.
Also, starting to reject such requests should be considered a breaking change. So overall I
am in favor of setting transaction type to custom in the server.

@axw
Copy link
Member Author

axw commented Jan 15, 2020

If agents simply set the custom type for such cases I think it would be better the APM Server accepts such requests and sets custom itself, as it is less user pain with the same outcome.

Are you suggesting that agents shouldn't set the type? I'm on the fence here. We've been tending not to defer too much logic to the server, and keep it in the agent.

Maybe we should do both? Set it in newer versions of the agents, and also in the server to cater for older agent versions. In a future version of the server we would tighten up the intake validation, and reject.

@simitt
Copy link
Contributor

simitt commented Jan 15, 2020

My take is that whenever the server can easily do something that all agents would need to implement, let's do it on the server. With easily I mean information that the server can retrieve from the information it has available, where no further context is needed that only the agents would have.
So what I meant was, implement it on the server, if the agent already also implements it fine, if not we still get the same result.

In a future version of the server we would tighten up the intake validation, and reject.

What value do you see in that, when the server is able to set the exact same value as the agents do?

@axw
Copy link
Member Author

axw commented Jan 15, 2020

In a future version of the server we would tighten up the intake validation, and reject.

What value do you see in that, when the server is able to set the exact same value as the agents do?

That statement was contingent on stating that an empty transaction type is invalid. i.e. it's invalid, but we'll allow it for now; later we won't. But that doesn't hold if we're going to say that it's valid for agents to omit it, and have the server fill it in.

It just occurred to me that agents probably need to be the ones to do it, as otherwise breakdown metrics won't be correct. We could have the server set those as well, but I think that's getting a bit messy.

With that in mind, how about we state that (for now) sending an empty transaction type leads to undefined behaviour, and make sure all agents are updated to prevent that. The server won't do anything to correct empty transaction types. Later on (e.g. in 8.0.0) we might reject such data.

@simitt
Copy link
Contributor

simitt commented Jan 15, 2020

Good point with the breakdown metrics. You suggestion SGTM, I added this issue to #2631 where we collect potential breaking changes for 8.0.

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

No branches or pull requests

3 participants