[exporter/awscloudwatchlogs] Feat: include scope in log record of cloudwatchlogs exporter #30316
Conversation
Include the instrumentation scope in the log records exported by the cloudwatchlogs expoter Signed-off-by: Raphael Silva <rapphil@gmail.com>
github-actions
bot
added
the
exporter/awscloudwatchlogs
Signed-off-by: Raphael Silva <rapphil@gmail.com>
Signed-off-by: Raphael Silva <rapphil@gmail.com>
rapphil
changed the title
| Attributes: attrsValue(scope.Attributes()), | ||
| } | ||
| body.Scope = scopeBody | ||
| } | ||
|
|
||
| bodyJSON, err = json.Marshal(body) |
There was a problem hiding this comment.
Reading this section of code made me wonder what would happen there was an attribute with Double type and Math.Inf() value.
With slightly modification to your PR I saw that the JSON marshaling would fail.
func testScope() pcommon.InstrumentationScope {
scope := pcommon.NewInstrumentationScope()
scope.SetName("test-scope")
scope.SetVersion("1.0.0")
scope.Attributes().PutStr("scope-attr", "value")
scope.Attributes().PutDouble("test-inf", math.Inf(0))
return scope
}
=== RUN TestLogToCWLog
=== RUN TestLogToCWLog/basic
exporter_test.go:249: logToCWLog() error = json: unsupported value: +Inf, wantErr false
--- FAIL: TestLogToCWLog (0.00s)
--- FAIL: TestLogToCWLog/basic (0.00s)
We have seen this JSON marshaling failure before in the EMF Exporter when a metric is processed with Inf or NaN values. In the EMF Exporter we completely drop the metric.
I think this "issue" is outside the scope of the PR but should at least be noted somewhere. At minumum I think this affects the awsemfexporter and awscloudwatchlogs exporter. I think this would also affect any exporter that does JSON marshaling before export. Having to clean a set of attributes for each log seems computationally expensive so we should verify the cost of doing so before moving forward with a very simple fix (checking for and removing all NaN/Inf) values.
bryan-aguilar
added
the
ready to merge
bryan-aguilar
added
ready to merge
…udwatchlogs exporter (open-telemetry#30316) **Description:** Include the instrumentation scope in the log records exported by the cloudwatchlogs expoter **Link to tracking Issue:** open-telemetry#29884 **Testing:** Unit tests were added. --------- Signed-off-by: Raphael Silva <rapphil@gmail.com> Co-authored-by: bryan-aguilar <46550959+bryan-aguilar@users.noreply.github.com>
Description: Include the instrumentation scope in the log records exported by the cloudwatchlogs expoter
Link to tracking Issue: #29884
Testing: Unit tests were added.