chore: Simplify RequestState/PositionalRequestArgs o11y code#63040
Conversation
kritzcreek
left a comment
There was a problem hiding this comment.
LGTM in general (thanks for doing this, I assume I "nerdsniped" you with my comment during our call)
Just have one question regarding the ordered-map dependency.
| package observation | ||
|
|
||
| import ( | ||
| orderedmap "github.com/wk8/go-ordered-map/v2" |
There was a problem hiding this comment.
Are we already depending on this somewhere else? I was surprised to see that a library for a datastructure also includes yaml and json parsers https://github.com/wk8/go-ordered-map/blob/85ca4a2b29d3241fa4513f82be3d38fe2392a791/go.mod#L5-L11
There was a problem hiding this comment.
It's for implementing deserialization from JSON and YAML, since the stdlib implementation for JSON parsing is slower. https://sourcegraph.com/github.com/wk8/go-ordered-map/-/blob/json.go?L109:9-109:19
There was a problem hiding this comment.
Are we already depending on this somewhere else?
Yes, but not in many places
There was a problem hiding this comment.
Well if we're already pulling it in I guess that's fine. I understand what it's used for, it just screams bad library at me. Probably just my intuitions being wrong in the context of Go again.
There was a problem hiding this comment.
It's similar in Rust, many libraries depend on serde via a crate feature. https://github.com/rust-random/rand/blob/master/Cargo.toml#L33
Go doesn't have support for feature flagging in the same way.
There was a problem hiding this comment.
Right, if it was feature flagged or in a "ordered-map-serialisation" library I wouldn't have flinched at all.
There was a problem hiding this comment.
It can't be in a separate library because you can't add methods to a type in a separate library (somewhat similar to the orphan rule), and the composable way for a type to be (de)serializable is by adding methods with specific signatures.
No, I was working on this locally before our call. |
The idea was for use in per-loop-iteration data point collection. Im not particularly happy with the implementation, Im also not sure if its actively being used anywhere. Bit of a hack that I wouldn't be against removing |
Moving the common observability attributes code into helper methods makes
reading the observation related code less painful.
TODO:
will end up creating a slice on seeing duplicate keys for the fields common between
RequestState and PositionalRequestArgs.
@Strum355, do you know why we keep multiple values for the same key instead
of having overwriting semantics? You added this behavior here:
https://github.com/sourcegraph/sourcegraph/pull/29652
Test plan
Covered by existing tests
Changelog