-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[chore] [exporter/splunk_hec] Remove redundant bufState struct #22986
Conversation
0a7d8c3
to
f33d85b
Compare
maxCapacity: bufCap, | ||
} | ||
} | ||
return &bufferState{ | ||
buf: buf, | ||
jsonStream: jsoniter.NewStream(jsoniter.ConfigDefault, nil, initBufferCap), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you changed the jsoniter config with this change - I can't remember if there is a reason for how it was set, just pointing that out. If tests pass, I guess we're ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right I changed it to jsoniter.ConfigFastest
unintentionally. Let me revert as it's not related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still have my approval either way. Since you're reverting that change, maybe worth updating the benchmark numbers real quick as well. I guess you'd want to also run a separate PR with the jsoniter changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the benchmarks appeared to be not as good without with the same jsoniter config. Maybe because of moving the jsoniter stream pool. Let me look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I changed it to fetch the stream from the pool once per batch. Benchmarks are good now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome. Do you want to file an issue for the jsoniter changes? @crobert-1 can make those if he's interested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jsoniter.ConfigFastest
doesn't escape html and drops precision after 6 decimal places. Do we want to accept that? If so, we can use this issue #22018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Splunk timestamps have a precision to the millisecond. I'm unsure about any other field that uses numbers specifically. I don't understand if escaping html is needed. I would be a cautious yes, maybe a feature gate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we need to check what UF is doing and keep the exporter behavior close to that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good conclusion. Let's get this one in and we can table the jsoniter changes until we have more tests against the UF.
f33d85b
to
d3f5fa1
Compare
d3f5fa1
to
7eec47e
Compare
…telemetry#22986) Most of the stuff was moved out from this struct, so it's not needed anymore.
Most of the stuff was moved out from this struct, so it's not needed anymore.
Before:
After: