-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix concurrency in emf exporter #2571
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2571 +/- ##
==========================================
+ Coverage 91.31% 91.32% +0.01%
==========================================
Files 429 429
Lines 21457 21467 +10
==========================================
+ Hits 19593 19605 +12
+ Misses 1395 1394 -1
+ Partials 469 468 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Can you summarize
Using channels could achieve the same goal, but we found it would make codes more complex to read to maintain the same semantics of the |
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.
LGTM! Thanks for the fixing.
if *logEvent.InputLogEvent.Timestamp == int64(0) { | ||
logEvent.InputLogEvent.Timestamp = aws.Int64(logEvent.LogGeneratedTime.UnixNano() / int64(time.Millisecond)) | ||
} | ||
return logEvent | ||
if len(*logEvent.InputLogEvent.Message) == 0 { | ||
return errors.New("empty log event message") | ||
} | ||
|
||
//http://docs.aws.amazon.com/goto/SdkForGoV1/logs-2014-03-28/PutLogEvents | ||
//* None of the log events in the batch can be more than 2 hours in the | ||
//future. | ||
//* None of the log events in the batch can be older than 14 days or the | ||
//retention period of the log group. | ||
currentTime := time.Now().UTC() | ||
utcTime := time.Unix(0, *logEvent.InputLogEvent.Timestamp*int64(time.Millisecond)).UTC() | ||
duration := currentTime.Sub(utcTime) |
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.
Thanks for putting all the data validation logic in one place.
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.
Small point, mostly LGTM
…pp (#2571) Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.35.0 to 1.36.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.35.0...v1.36.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Why do we need it?
This PR fixes the concurrency issue described in aws-observability/aws-otel-collector#387, and does a code refactoring to
pusher
.We add two locks, one for
logEventBatch
instance update and one forstreamToken
replacement to ensure they're both goroutine safe. It also fixes an issue of deciding where a new logEvent should go to when the current batch could not meet the requirement before or after the new entry appending. In today's implementation, it is added to the old batch, and this PR changes it to be added to a newly created batch.