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

Elasticsearch exporter: Init JSON encoding support #3101

Merged

Conversation

urso
Copy link

@urso urso commented Apr 14, 2021

Description: The change adds support for encoding OpenTelemetry log records to JSON.
The encoder tries to remove duplicate entries in case the attribute map
(which is an array if key value pairs) contains duplicates.

Mixed style attributes, with key names having dots and other fields
having attribute maps as value will be normalized, such that the JSON
encoding will be either completely flat, or values are properly merged
into a single JSON object (when dedot is enabled). The normalization
helps with deduplication, and (not yet implemented) dedot support will
allow us to present a well formated JSON event if Ingest Node is used
(The dedotting in Elasticsearch does happen after Ingest Node).

Next:

  • Dedotting support
  • Custom (configurable) field mapping
  • publishLogs unit testing
  • Integration tests

Link to tracking Issue: #1800

Testing:
The internal document type with field deduplication is fully tested (89%) via unit tests.

The change also hooks up publishLogs, but this functionality is not covered by tests yet, as the PR has already grown quite a bit (I tested locally with a custom otel collector distribution). I would like to add additional tests in a separate PR, to keep focused on the JSON encoding only here.

Documentation: None.

@urso urso requested a review from a team April 14, 2021 11:34
@urso
Copy link
Author

urso commented Apr 14, 2021

@faec Can you help reviewing please?

@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #3101 (8d4210f) into main (17399fa) will decrease coverage by 0.13%.
The diff coverage is 70.30%.

❗ Current head 8d4210f differs from pull request most recent head a4a27ea. Consider uploading reports for the commit a4a27ea to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3101      +/-   ##
==========================================
- Coverage   91.91%   91.77%   -0.14%     
==========================================
  Files         494      496       +2     
  Lines       23939    24103     +164     
==========================================
+ Hits        22003    22120     +117     
- Misses       1429     1472      +43     
- Partials      507      511       +4     
Flag Coverage Δ
integration 63.74% <ø> (+0.05%) ⬆️
unit 90.79% <70.30%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/elasticsearchexporter/model.go 0.00% <0.00%> (ø)
exporter/elasticsearchexporter/exporter.go 75.00% <11.76%> (-8.34%) ⬇️
...lasticsearchexporter/internal/objmodel/objmodel.go 87.02% <87.02%> (ø)
processor/groupbytraceprocessor/event.go 96.77% <0.00%> (+0.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67e71dd...a4a27ea. Read the comment docs.

@urso urso force-pushed the esexporter-init-json-encoding branch from 8583a90 to 059f38d Compare April 16, 2021 13:21
@tigrannajaryan
Copy link
Member

@faec @blakerouse please review as codeowners.

@tigrannajaryan
Copy link
Member

@urso please resolve the merge conflict.

@tigrannajaryan
Copy link
Member

@faec @blakerouse please review.

Copy link

@faec faec left a comment

Choose a reason for hiding this comment

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

Looks good, mostly just some corrected typos, but also some questions about corner cases in deduping -- I'm approving since I think there'll be no problem addressing them, but I'm happy to rereview if there is complexity in the followup.

@bogdandrutu
Copy link
Member

@urso please mark as resolved conversations that are resolved so easier to check the status. Please rebase

@urso urso force-pushed the esexporter-init-json-encoding branch from 4bfa870 to 0462351 Compare May 5, 2021 20:54
@urso
Copy link
Author

urso commented May 5, 2021

@bogdandrutu Done.

@urso urso force-pushed the esexporter-init-json-encoding branch 3 times, most recently from 8d6f9be to 1a80aa9 Compare May 8, 2021 19:28
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 16, 2021
@urso urso force-pushed the esexporter-init-json-encoding branch 2 times, most recently from e0d4b80 to 3ca7017 Compare May 18, 2021 11:10
@urso urso closed this May 18, 2021
@urso urso reopened this May 18, 2021
@urso
Copy link
Author

urso commented May 18, 2021

@bogdandrutu Any idea about the CI errors? Due to merge conflicts in go.mod every now and then I'm rebasing the PR over and over again. Recently CI fails in windows-test and loadtest, but without any output at all.

@github-actions github-actions bot removed the Stale label May 19, 2021
@tigrannajaryan
Copy link
Member

Please rebase from main. It should fix the build.

The change adds support for encoding OpenTelemetry log records to JSON.
The encoder tries to remove duplicate entries in case the attribute map
(which is an array if key value pairs) contains duplicates.

Mixed style attributes, with key names having dots and other fields
having attribute maps as value will be normalized, such that the JSON
encoding will be either completely flat, or values are properly merged
into a single JSON object (when dedot is enabled). The normalization
helps with deduplication, and (not yet implemented) dedot support will
allow us to present a well formated JSON event if Ingest Node is used
(The dedotting in Elasticsearch does happen after Ingest Node).

Next:
- Dedotting support
- Custom (configurable) field mapping
- Testing testing
@urso urso force-pushed the esexporter-init-json-encoding branch from 3ca7017 to a4a27ea Compare May 19, 2021 20:24
@urso
Copy link
Author

urso commented May 20, 2021

@tigrannajaryan @bogdandrutu Rebased yesterday. CI is green now. Thanks for fixing the CircleCI configs.

@tigrannajaryan tigrannajaryan merged commit 9404a13 into open-telemetry:main May 20, 2021
@tigrannajaryan
Copy link
Member

If this fully resolves #1800 please close it.

@urso urso deleted the esexporter-init-json-encoding branch May 25, 2021 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants