Skip to content
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

scope -> name missing in cloudwatch logs exporter #29884

Closed
MaGaudin opened this issue Dec 14, 2023 · 11 comments
Closed

scope -> name missing in cloudwatch logs exporter #29884

MaGaudin opened this issue Dec 14, 2023 · 11 comments
Labels
bug Something isn't working exporter/awscloudwatchlogs awscloudwatchlogs exporter help wanted Extra attention is needed Stale

Comments

@MaGaudin
Copy link

MaGaudin commented Dec 14, 2023

Component(s)

No response

What happened?

Description

Our ADOT Collector is receiving this log:
{
"resourceLogs": [
{
"resource": {
"attributes": [...]
},
"scopeLogs": [
{
"scope": {
"name": "xxxxxxx"
},
"logRecords": [
{
"timeUnixNano": "xxx",
"observedTimeUnixNano": "xxxx",
"severityNumber": 9,
"severityText": "Information",
"body": {
"stringValue": "GET / \r\n"
},
"attributes": [...],
"traceId": "xxxx",
"spanId": "xxxx"
}
]
}
]
}
]
}

but in AWS Cloudwatch we are not able to see the field scope.name. We saw from the file exporter.go (https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/awscloudwatchlogsexporter/exporter.go) that is missing the logic to retrieve the field from line 143-146.

Steps to Reproduce

Expected Result

We would like to have this field in our cloudwatch logs.

Actual Result

this field is missing

Collector version

v0.90.1 for OTEL and v0.36.0 for ADOT

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

@MaGaudin MaGaudin added bug Something isn't working needs triage New item requiring triage labels Dec 14, 2023
@sgarzo10
Copy link

I have the same problem

@songy23 songy23 added the exporter/awscloudwatchlogs awscloudwatchlogs exporter label Dec 14, 2023
Copy link
Contributor

Pinging code owners for exporter/awscloudwatchlogs: @boostchicken @bryan-aguilar @rapphil. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

Hello @MaGaudin, I was able to confirm your findings and agree the scope and scope name should be included in the log being sent to CloudWatch. Code owners can correct me if there's a specific reason this should not be included.

Solution proposal:

The exporter is essentially copying all data it wants to send into a local struct called cwLogBody, which will then marshal all of the information into JSON. This JSON will then be converted to a string called Message.

I believe the solution here is to modify the function signature to accept plog.Scope as well here:

func logToCWLog(resourceAttrs map[string]any, log plog.LogRecord, config *Config) (*cwlogs.Event, error) {

We can get Scope from sl.Scope here:

cwLogBody would also need to be modified to introduce scope here:

Then the scope information could just be added here alongside resource:

Note:
At some point the code owners here may want to evaluate moving to a well-defined central marshaller for logs. I see there are some specific formatting changes being done, but having to manually copy all the data and then marshall what you've copied is prone to errors and hard to maintain. Something like what the ExporterHelper does, if that works for the use case here.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Dec 14, 2023
@crobert-1
Copy link
Member

crobert-1 commented Dec 14, 2023

One thing to note: My proposed solution will not work when the configuration option raw_logs is set to true. I'll defer to code owners on how to handle that case, and if handling it is necessary.

@MaGaudin
Copy link
Author

Thank you so much for the propose solution @crobert-1! So, we need to wait for an opinion from the code owners?

@boostchicken @bryan-aguilar @rapphil

@crobert-1
Copy link
Member

You're welcome to post a PR with the change. Otherwise, yes, you'll need to wait for code owners or other contributors to provide a fix.

@bryan-aguilar
Copy link
Contributor

Thank you for your investigation @crobert-1, it was very thorough and helpful. I agree with everything that has been said so far. I believe it would make sense for the InstrumentationScope to be included.

My proposed solution will not work when the configuration option raw_logs is set to true.

raw_log should really be looked through as a passthrough mode with a little logic to handle EMF messages. I think it would be okay to ignore this case.

At some point the code owners here may want to evaluate moving to a well-defined central marshaller for logs.

Agreed, this would be nice if it could fit nicely into the existing CWL logic, but I do not think and would not suggest for whoever takes on this PR to tackle this now. I would request a separate PR.

@bryan-aguilar bryan-aguilar added the help wanted Extra attention is needed label Dec 19, 2023
@MaGaudin
Copy link
Author

MaGaudin commented Dec 19, 2023

Hi @bryan-aguilar thanks for your feedback. How could we request for a separate PR? is there any possibility to unblock this feature?

@bryan-aguilar
Copy link
Contributor

I was suggesting to have separate PRs for the instrumentation scope pr and new, consistent marshaling behavior. This PR is not blocked and awaiting for a contributor to pick up the work.

bogdandrutu pushed a commit that referenced this issue Jan 10, 2024
…udwatchlogs exporter (#30316)

**Description:** Include the instrumentation scope in the log records
exported by the cloudwatchlogs expoter

**Link to tracking Issue:** #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>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
…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>
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Feb 19, 2024
@crobert-1 crobert-1 removed the Stale label Feb 26, 2024
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exporter/awscloudwatchlogs awscloudwatchlogs exporter help wanted Extra attention is needed Stale
Projects
None yet
Development

No branches or pull requests

5 participants