Skip to content
This repository was archived by the owner on Aug 17, 2020. It is now read-only.

Conversation

@drodriguezhdez
Copy link
Contributor

This PR changes the way of generating random opentracing IDs to use a random pool, which improves the spread when random are obtained across multiple goroutines.

@drodriguezhdez drodriguezhdez self-assigned this Apr 14, 2020
@tonyredondo
Copy link
Contributor

tonyredondo commented Apr 14, 2020

I don't think this implementation could help improve the collision problem, It seems to help with concurrency and performance of the concurrency due the pool + mutex of different rands. But nothing about collision.

Maybe we can do a collision tests with this implementation vs normal math/rand and vs crypto/rand. I think the crypto/rand implementation would have fewer collisions.

Beside using a better random generator, the problem will not be solved using this number of bits in the Id, In the backend the tests executions and span count keep growing, so the probability of a collision keeps increasing... I don't know if the backend could have a composed primary key, like traceId + parentSpanId + spanId. This could reduce the probability of a collision drastically. Or wait until the Otel implementation...

What do you think @fermayo

@fermayo
Copy link
Member

fermayo commented Apr 14, 2020

Apart from what we can do in the backend, we are having an abnormally high collision rate of span IDs in the case of the Go agent.

@tonyredondo
Copy link
Contributor

Apart from what we can do in the backend, we are having an abnormally high collision rate of span IDs in the case of the Go agent.

Across the same traceId? Because this also will not help in differents packages/modules

In the same process I guess I prefer the crypto implementation, less code changes... https://golang.org/pkg/crypto/rand/

@tonyredondo
Copy link
Contributor

The better is to create a test comparing all the random generators and select the one with fewer collisions.... I guess

@drodriguezhdez
Copy link
Contributor Author

drodriguezhdez commented Apr 14, 2020

@tonyredondo you're right, this PR seems does not improve the collision rate, it's just for performance. My bad.

Regarding using crypto/rand, I have my doubts because of the additional overhead that could suppose that to the agent. Other opentracing agents (e.g. Lightstep) are using math/rand.

Let's test it as @tonyredondo said, but I think the problem is not in the random generator, but in the bits of the ID that we're using. I'm wondering if we're obtaining that collision rate in Go agent cause it is the most used agent in Scope (practically all customers use Go).

@drodriguezhdez
Copy link
Contributor Author

Closed due to #223

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants