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

[jsonlogencodingextension] Add Modes for Body with Attributes #32722

Merged
merged 18 commits into from
May 7, 2024

Conversation

blakeromano
Copy link
Contributor

Description:

The goal of this PR is to add configuration to the JSON Encoding method to have metadata from resources and attributes in logs and output those into a json attribute. This also will output the body of the log in the body attribute. This follows a similar pattern that the AWS Cloudwatch Log Exporter uses but removes any of the AWS specific logic and makes this a more generalized solution

Link to tracking Issue: #32679

Testing: Unit Tests were added to validate that the output is desired and that the existing logic remains the same.

Documentation: Documentation regarding the usage of the configuration as well as the output to expect is added.

@blakeromano blakeromano requested a review from atoulme as a code owner April 26, 2024 21:14
@blakeromano blakeromano requested a review from a team April 26, 2024 21:14
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

I am ok with this as an alternative to otlp json encoding. Noting outputting the resource attributes for each record is of course wasteful, I suppose you meant for this to be a self sufficient log representation

@blakeromano
Copy link
Contributor Author

blakeromano commented Apr 28, 2024

I am ok with this as an alternative to otlp json encoding. Noting outputting the resource attributes for each record is of course wasteful, I suppose you meant for this to be a self sufficient log representation

If I understand your comment enough that is correct. The idea is each index in the array is a self contained log. The hope is that this provides a fairly stable format for ingestion to be able to parse into a usable format. The other alternative I guess that I wasn't sure if it was possible would be to have a seperate return for each log record therefore instead of an array of items you'd have

{
    "body": "log testing",
    "resource": {
      "test": "logs-test"
    }
  }

which I think may be desirable however I am not sure the value add is worth it. I think if anything a future enhancement to the encoder could be a "split" mode where a user could define whether they'd want to split it into two separate log entries or have it combined into one but I feel like that may not be an encoder issue rather a processor problem.

We could also have a seperate mode later on for folks who do not want to have resource attributes in each document and instead have it once and then have each body in an array something like

{
  "body": [
    "foo",
    "bar",
  ],
  "resource": {
      "test": "logs-test"
    }
}

@blakeromano blakeromano requested a review from atoulme April 28, 2024 19:06
extension/encoding/jsonlogencodingextension/extension.go Outdated Show resolved Hide resolved
Comment on lines 104 to 105
Attributes map[string]any `json:"attributes,omitempty"`
Resource map[string]any `json:"resource,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe log_attributes and resource_attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of trying to stay with camelCase as most json is but I wouldn't be opposed to logAttributes and resourceAttributes

@blakeromano blakeromano changed the title Pretty JSON Encoding [jsonlogencodingextension] Pretty JSON Encoding Apr 28, 2024
@blakeromano blakeromano changed the title [jsonlogencodingextension] Pretty JSON Encoding [jsonlogencodingextension] Add Modes for Body with Attributes Apr 28, 2024
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM with 2 nits.

@atoulme
Copy link
Contributor

atoulme commented Apr 29, 2024

BTW, your approach discards scope log attributes, I'm not sure if that matters.

@blakeromano
Copy link
Contributor Author

BTW, your approach discards scope log attributes, I'm not sure if that matters.

I don't think it matters but I am happy to hear other opinions

@blakeromano blakeromano requested a review from dmitryax April 30, 2024 02:29
@blakeromano
Copy link
Contributor Author

@atoulme @dmitryax all the tests should pass now. Let me know if there's anything else needed on my side to see this get merged.

Copy link
Contributor

@tiffany76 tiffany76 left a comment

Choose a reason for hiding this comment

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

Just a few grammar and style suggestions from a docs perspective. Thanks!

extension/encoding/jsonlogencodingextension/README.md Outdated Show resolved Hide resolved
extension/encoding/jsonlogencodingextension/README.md Outdated Show resolved Hide resolved
extension/encoding/jsonlogencodingextension/README.md Outdated Show resolved Hide resolved
@blakeromano
Copy link
Contributor Author

I believe I addressed all the naming and doc suggestions thank you for that feedback! I am not sure why there was so many status failures on the last commit. A lot of them seemed relating to something else so not sure it is because of this PR. I was able to validate linting and tests for this component were all passing.

@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label May 3, 2024
@blakeromano
Copy link
Contributor Author

Hey folks just wondering what the next steps are to get this merged? 😄

{
"body": "log testing",
"resource": {
Copy link
Member

Choose a reason for hiding this comment

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

This should be resourceAttributes I'm guessing?

Suggested change
"resource": {
"resourceAttributes": {

### Mode

#### body Mode
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to make the headings consistent

Suggested change
#### body Mode
#### body

Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

It would be great if you could fix the small docs remarks in a follow-up PR @blakeromano

The component is in development stability, so it's probably better to move fast in small increments rather than sweat over every little detail.

@andrzej-stencel andrzej-stencel merged commit f22eadd into open-telemetry:main May 7, 2024
166 of 167 checks passed
@github-actions github-actions bot added this to the next release milestone May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension/encoding ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants