automatic instrumentation does not override manual results

[wolframhaussig]

This PR changes the logic of the result so results set via the API takes precedence. Until now, a result set via the API would be overridden by the automatic instrumentation(for example on a HTTP call).

I have removed the check in the quartz plugin which I have added while writing it because it did the same thing.

Checklist

• Implement code
• Add tests

[eyalkoren]

@wolframhaussig Thanks for the PR!

Since our auto instrumentation would normally set the result as the last thing before ending the transaction, I wonder if it would be good enough to change the implementation of Transaction#withResult to set the result field only if it is still null.
Since our injected code must run after the application code (which includes the manual API usage) - this should be safe enough.

[wolframhaussig]

@eyalkoren I guess I thought too compliated here 😄 I thought that as you have already a ticket for priority of transaction names that there might be the usecase to have the priority here too... I will change the code as suggested

[eyalkoren]

@wolframhaussig you are absolutely right, there can definitely be a case for that in the future, but your implementation basically allows two priorities (user-defined or other), so I think it might as well be done with a null check. If we encounter a situation where we need multiple priorities for the result in the future- we can add it then.

[wolframhaussig]

@eyalkoren I have found a code piece where we might change the result - can you have a look at that and tell me your opinion? In the opentracing implemenation there is this method.
Now, if there would be an error key it would initiate the result and if there is a result tag afterwards the result would not be overridden anymore. Is this usecase possible with opentracing - I have no experience with that?

[eyalkoren]

@wolframhaussig yes, you are right, in this case you could assign higher priority to the result tag than the error tag. However, we could say that about other data as well I guess, not sure we want to over-complicate things with lots of priorities.
@felixbarny what do you think?

[felixbarny]

Until now, a result set via the API would be overridden by the automatic instrumentation

That could be fixed by checking if the result has already been set in the instrumentation.

Now, if there would be an error key it would initiate the result and if there is a result tag afterwards the result would not be overridden anymore.

Good catch.

We could have two methods like setResult and setResultIfUnset, or similar and call the appropriate method for the situation:

• Instrumentation code and the error tag call setResultIfUnset
• Setting the result explicitly via the API or the result tag should call setResult which may override the result.

[felixbarny]

run the tests