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

[awss3exporter]: add Sumo Logic marshaler for logs in Installed Collector format #23649

Merged

Conversation

aboguszewski-sumo
Copy link
Member

Description: This PR adds a new marshaller for awss3 exporter. It exports logs in the format of Sumo Logic Installed Collector. Metrics and traces are not supported - creating an exporter for them will result in an error.
Currently, nested typed (eg. map inside a map) might not be supported correctly - I have to confirm the IC's behavior with them, but I wanted to create the PR so that it can be reviewed early.

Link to tracking Issue: #23212

Testing: Unit tests and manual e2e tests. Some automatic e2e tests will come later, but they will not be part of this repo, they will be a test for integrating the ingest with Sumo Logic's backend.

Documentation: Readme updated.

@aboguszewski-sumo aboguszewski-sumo requested a review from a team June 23, 2023 11:17
@aboguszewski-sumo aboguszewski-sumo changed the title [awss3exporter]: add Sumo Logic marshaller for logs in Installed Collector format [awss3exporter]: add Sumo Logic marshaler for logs in Installed Collector format Jun 23, 2023
@github-actions github-actions bot requested a review from pdelewski June 23, 2023 11:18
dateVal := lr.ObservedTimestamp()
body := attributeValueToString(lr.Body())

logEntry(&buf, "{\"date\": \"%s\",\"sourceName\":\"%s\",\"sourceHost\":\"%s\",\"sourceCategory\":\"%s\",\"fields\":{},\"message\":\"%s\"}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is going to produce valid JSON? What if I have source field with the value "foo"bar" ? Can you please add a test with that example?

I note you're also dropping all resource and log attributes, is that expected or did I miss something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's expected to drop these, but I'll confirm that with others.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After verifying the format, I've decided to do the following:

  • replace custom field format with JSON and quote it
  • place all resource attributes that are not source* into fields
  • merge log record-level attributes into one JSON with the body
    Sorry for the mess, but it should be clearer now.

@aboguszewski-sumo
Copy link
Member Author

@atoulme sorry for the mess, but I've decided to rework some things in this PR. It's ready for another review.

@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Jul 20, 2023
@aboguszewski-sumo aboguszewski-sumo force-pushed the awss3exporter-add-sumo-logs-marshaller branch from 5543a19 to fb2d997 Compare July 20, 2023 08:26
@aboguszewski-sumo
Copy link
Member Author

One small change before merging: the file format was changed to json.gz (although we don't compress here, but the extension is enough in this usecase)

@mx-psi
Copy link
Member

mx-psi commented Aug 1, 2023

needs a rebase

@TylerHelmuth TylerHelmuth removed the ready to merge Code review completed; ready to merge by maintainers label Aug 4, 2023
@aboguszewski-sumo aboguszewski-sumo force-pushed the awss3exporter-add-sumo-logs-marshaller branch from 9277a8b to d084d23 Compare August 7, 2023 08:30
@aboguszewski-sumo
Copy link
Member Author

@mx-psi @atoulme rebased.

@mx-psi mx-psi merged commit 74cffb5 into open-telemetry:main Aug 7, 2023
@github-actions github-actions bot added this to the next release milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants