Conversation
antonireus
left a comment
There was a problem hiding this comment.
In general the code is quite good. The code for "closing" feels a bit odd at the begining, but looking at the specs this arises for the requirment of the shutdown process has to send the remaining messages.
The only issue would be that for a telemetry library I think we should aim at having a lower overhead. Some would label this as "premature optimization", but I think there are some good practives that should be followed to avoid some unnecessary allocations and computations, that wouldn't make the code less readable.
Perhaps, what is missing is a performance requirement in the speq, something like the library should have a low overhead, and avoid memory allocations if possible.... and when tracing is disabled the tracing should be a no-op wihtout overhead.
Also clarifying what can be passed as feature would help. This will likely be constant strings, whitout special characters, so it doesn't make sense that the library spends compute with trimming calls, and making copyies to escape chars that won't be there.
Finally, on the test side, I'm missing tests to verify that the JSON payloads are wellformed.
|
@antonireus I created a separate issue for validating feature names, as it's not yet clear how to implement this: #4 |
Closes #2