This repository was archived by the owner on Sep 30, 2024. It is now read-only.
honey: replace no-op event with new NonSendingEvent that stores fields in memory#61854
Merged
Conversation
878ab53 to
885b541
Compare
ggilmore
commented
Apr 13, 2024
Contributor
Author
There was a problem hiding this comment.
We don't use the Add function anywhere in the codebase. Simply removing this from the interface means that we don't have to re-implement this unnecessarily.
eseliger
approved these changes
Apr 13, 2024
Comment on lines
30
to
37
Member
There was a problem hiding this comment.
does the event need to be concurrency safe? would the real event be safe?
Contributor
Author
There was a problem hiding this comment.
Ah, that's a good call out. Funnily enough, the event type from libhoney is threadsafe - but I think our eventwrapper isn't 🙃
I'll wrap a mutex around this anyway.
bb27938 to
b5c0cd1
Compare
…s in memory commit-id:12b8f3ad
b5c0cd1 to
cbc178b
Compare
Comment on lines
+34
to
+42
| var wg sync.WaitGroup | ||
| for i := 0; i < 1000; i++ { | ||
| wg.Add(1) | ||
| go func(i int) { | ||
| defer wg.Done() | ||
| event.AddField(fmt.Sprintf("key%d", i), i) | ||
| }(i) | ||
| } | ||
| wg.Wait() |
Contributor
There was a problem hiding this comment.
This kind of code would be slightly simpler with our conc library.
wg := conc.NewWaitGroup()
for i := 0; i < 1000; i++ {
wg.Go(func() {
event.AddField(fmt.Sprintf("key%d", i), i)
})
}
wg.Wait()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: https://github.com/sourcegraph/sourcegraph/issues/60726
When honeycomb wasn't enabled, we use a noop Event type everywhere. However, there are some places where we'd like to inspect or log the event regardless of whether or not honeycomb is enabled, like the following code in gitserver.
https://github.com/sourcegraph/sourcegraph/blob/ee9199762853c10e6a737f341ceb0808ceca20ea/cmd/gitserver/internal/git/gitcli/command.go#L280-L286
Our use of the noop type, resulted in unexpected behavior - such as the
ev.Fieldsin the log line not getting populated (https://github.com/sourcegraph/sourcegraph/issues/60726).This PR fixes this issue by replacing the noop event type a new NonSendingEvent which still allows
Fieldsto be aggregated in memory and later retrieved withFields(). This ensures that use cases like the logging one above still work as expected.Test plan
Unit tests
Also saw the following logs when I ran sourcegraph with
SRC_DEVELOPMENT=false SRC_LOG_FORMAT=json sg start(notice thatev.Fieldsis no-longer null).