tracer: Drop unsampled txs when APM Server >= 8.0 #1208
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
Extends the HTTP Transport with a new method that queries the remote APM Server `/` endpoint to obtain its version. This allows the tracer to drop unsampled transactions when the APM Server to which it sends traces is version `8.0.0` or higher. The version will be cached in a local variable until the APM Server URL changes which will refresh the APM Server's version. Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
transport/http.go
Outdated
| @@ -306,7 +313,10 @@ func (t *HTTPTransport) SetServerURL(u ...*url.URL) error { | |||
| t.intakeURLs = intakeURLs | |||
| t.configURLs = configURLs | |||
| t.profileURLs = profileURLs | |||
| t.urlIndex = 0 | |||
| atomic.StoreInt32(&t.urlIndex, 0) | |||
There was a problem hiding this comment.
There was an unwritten assumption that SetServerURLs is not called concurrently with other methods: t.intakeURLs etc. are set without a lock. Is there some new concurrency that requires the atomic call?
There was a problem hiding this comment.
Since it wasn't stated anywhere and it seemed like a fairly small change to avoid having a potential data race when the index is increased and SetServerURL is called at the same time.
I can revert if you feel like we don't need to do it, but it seemed like a small price to pay to avoid any potential panics on consumer applications.
There was a problem hiding this comment.
I'd prefer to be precise when it comes to concurrency: either the method needs to be concurrent-safe, or it doesn't. The atomic implies it does, but the unsynchronised mutation of t.intakeURLs etc. implies it doesn't.
Unless we need it (maybe we do?), I'd prefer to revert.
There was a problem hiding this comment.
I see, I will revert the change since it's unclear whether this is a scenario that users may encounter.
Signed-off-by: Marc Lopez Rubio <[email protected]>
transport/http.go
Outdated
| @@ -306,7 +313,10 @@ func (t *HTTPTransport) SetServerURL(u ...*url.URL) error { | |||
| t.intakeURLs = intakeURLs | |||
| t.configURLs = configURLs | |||
| t.profileURLs = profileURLs | |||
| t.urlIndex = 0 | |||
| atomic.StoreInt32(&t.urlIndex, 0) | |||
There was a problem hiding this comment.
I'd prefer to be precise when it comes to concurrency: either the method needs to be concurrent-safe, or it doesn't. The atomic implies it does, but the unsynchronised mutation of t.intakeURLs etc. implies it doesn't.
Unless we need it (maybe we do?), I'd prefer to revert.
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Description
Extends the HTTP Transport with a new method that queries the remote APM
Server
/endpoint to obtain its version. Allowing the tracer to dropunsampled transactions when the APM Server is version
8.0.0or higher.The version will be cached in a local variable until the APM Server URL
changes which will refresh the APM Server's version. The tracer launches
a goroutine to update the version when it is instantiated and refreshes
the remote server version every 10 seconds.
Any SendStream errors will cause the version cache to be invalidated and
will be refreshed the next time the ticker runs. It may cause unsampled
to be sent to the APM Server while the cache is invalid.
Related Issues
Closes #1153