Extract CWlogs client from EMF exporter and use in Awscloudwatchlogsexporter #6969
Conversation
…nent to push logs, create log group/stream in cloudwatch
| seqTokenMu sync.Mutex | ||
| seqToken string | ||
| groupStreamToPusherMap map[string]map[string]cloudwatch.Pusher |
There was a problem hiding this comment.
This is overly verbose name, is there any way to make this concise?
| if batch.minTimestampMs == 0 || batch.maxTimestampMs == 0 { | ||
| return true | ||
| } | ||
| if *targetTimestampMs-batch.minTimestampMs > 24*3600*1e3 { |
There was a problem hiding this comment.
Please avoid using magic numbers
| // log part and retry logic are already done inside the CreateStream | ||
| // when the error is not nil, the stream token is "", which is handled in the below logic. | ||
| p.streamToken, err = p.svcStructuredLog.CreateStream(p.logGroupName, p.logStreamName) | ||
| // TODO Known issue: createStream will fail if the corresponding logGroup and logStream has been created. |
There was a problem hiding this comment.
There is no todo, only dos in my reviews.
| // TODO Known issue: createStream will fail if the corresponding logGroup and logStream has been created. | ||
| // The retry mechanism helps get the first stream token, yet the first batch will be sent twice in this situation. | ||
| if err != nil { | ||
| p.logger.Warn("Failed to create stream token", zap.Error(err)) |
There was a problem hiding this comment.
This should probably fail the method right?
internal/aws/cloudwatch/pusher.go
Outdated
|
|
||
| var tmpToken *string | ||
| var err error | ||
| p.logger.Info("PUTTING LOG EVENTS") |
There was a problem hiding this comment.
Please move this to debug, and use normal casing.
internal/aws/cloudwatch/pusher.go
Outdated
|
|
||
| tmpToken, err = p.svcStructuredLog.PutLogEvents(putLogEventsInput, p.retryCnt) | ||
|
|
||
| p.logger.Info("tmp token is %v", zap.String("tmpToken", *tmpToken)) |
There was a problem hiding this comment.
Please don't use a formatted string for logging.
|
This is a very large PR @aateeqi , Please consider breaking this down into smaller PRs |
I discussed this with @aateeqi last week and I think this is the first of several steps that will be necessary to unify the EMF and CW logs exporters:
@aateeqi can you please ensure that there is an issue that documents the high-level plan and the steps involved?
Perhaps, though I think the bulk of it is in the |
|
That makes a bit more sense @Aneurysm9 , There was a lot that was changing so it was hard to rationalise what was a lift and shift, and what was new behaviour. +1 on an issue describing the changes, it simply helps me review and understand what is the expected behaviour and end state just to help validate it does what is expected. If it is also not too much to ask, a link to any public references would also be appreciated to help understand the changes. I feel like I am asking for a lot but I would like to make sure I understand in enough detail to help get these changes through. |
|
Hey @MovieStoreGuy, thanks for taking a look at the PR and leaving some insightful comments and @Aneurysm9 for also explaining some of the high level plan. The PR is still in draft stage and was published mainly for visibility and I generally agree with whats being said and will be continually updating. |
|
It is honestly a pleasure @aateeqi , I am very happy to review things and help if needed. If you'd like any further reviews, just ping me and I shall check it within the day (timezone nonsense and all that). Happy and safe holidays to you. |
aateeqi
changed the title
|
This PR seems to be hard to review since it's doing it in the wrong order. We should "Extract CWlogs client from EMF exporter and use in awsemfexporter". This is "Copy CWLogs client from EMF exporter to internal and use in logs exporter". It causes all of the pusher related code to be treated as new files instead of moved code. Additionally, we can't gauge the impact this change will have on the emf exporter, which is actually the more important component when extracting code from it. Can we close this PR and start with the emfexporter instead? |
Description:
Currently the awscloudwatchlogs exporter is missing some desired functionality based on the issues linked below. Since the
awsemfexporteralready supports some of this functionality, this PR copies and packages this emf logic into acloudwatchinternal component to push to cloudwatch logs.The follow-up of this PR would be to hook up the
awsemfexporterto also call thecloudwatchpackage to push metrics to cloudwatch.cloudwatchinternal component that supports:- automatically creating log group and log stream on push
- batch logs to 256 kb
Link to tracking Issue:
#6991
Testing:
built and ran the otel agent with the following that reads from a local file (/var/log/depnotify.log) and verified that the logs were pushed to cloudwatch with no errors
`receivers:
filelog:
start_at: beginning
include: [ /var/log/depnotify.log ]
otlp:
protocols:
http:
grpc:
endpoint: 0.0.0.0:4317
exporters:
logging:
awsxray:
region: us-west-2
awscloudwatchlogs:
log_group_name: test-sample-group
log_stream_name: test-sample-stream
region: us-west-2
service:
telemetry:
logs:
pipelines:
logs:
receivers:
- otlp
- filelog
exporters:
- awscloudwatchlogs`
Documentation:
TODO