add node_id to emited spans #5985
Conversation
| } | ||
| }() | ||
| .await | ||
| .unwrap_or_else(|| "unknown".to_string()); |
There was a problem hiding this comment.
should the default value contain a random number in it?
There was a problem hiding this comment.
i think it won't be useful because the 2 situations where this is emitted should be:
- the command is not one that behave like a node at all
- we couldn't read the config file, and assuming that stays true, the node will crash shortly
but there is little harm at doing it, so why not
| // Except for race conditions, an error will get logged the 2nd time the config is read, | ||
| // inside the command. In case of race condition, we just don't know the node id for tracing | ||
| // purpose | ||
| let node_id = { |
There was a problem hiding this comment.
I see. I suggest we do simpler... but less correct.
Just use the QW_NODE_ID env variable here.
It won't capture people using the node_id config value, and it will break if we change the helm chart, but it has the merit of not adding the extra complexity in this PR (loading the config twice, etc.)
There was a problem hiding this comment.
i prefer reading the config: not all deployments of quickwit uses an helm chart
i'd argue that our configuration loading process is complexe, but well encapsulated, so loading the config twice doesn't harm code maintainability
|
This is a recurring pattern and issue. We want to emit some logs / metrics / traces containing info from the node config but it is loaded after setting up, sometimes immutable (hello Can we consider:
I know it sucks to have to do this refactor when we just wanted to add a new tag but that would make the codebase healthier. |
|
We can also ensure the trace is set up with |
Description
add the node_id to emitted spans, making traces easier to interpret
How was this PR tested?
spin a one node cluster, make it trace to itself, and verify the new field is shown in jaeger