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

Add Elasticsearch Log Exporter with tests #444

Merged
merged 20 commits into from
Dec 22, 2020

Conversation

MarkSeufert
Copy link
Contributor

This PR adds an Elasticsearch exporter for logs, which is a requirement from issue #337. The main features include:

  • Specifying the host, port, and index/data stream of the Elasticsearch instance
  • Sending an HTTP POST request to Elasticsearch using the HTTP Client Interface, and parsing the response message to ensure it was successfully received
  • Sending batches of LogRecords using the Bulk API
  • Timeout logic, such that the Export() method doesn't block indefinitely
  • Unit tests for the above functionality

Before reviewing this PR, it should be noted that:

  • This exporter current does not support CMake build as there is currently no CMake support for the Nlohmann/JSON library - will try to find a way around this using a backport for it (cc @maxgolov)
  • Changes to logger.h and log_record.h can be ignore, as this PR will soon be changed to use a recordable instead of an API LogRecord once the recordable PR gets merged.

cc - @xukaren @alolita @lalitb @jsuereth @ThomsonTan

@MarkSeufert MarkSeufert requested a review from a team December 13, 2020 22:35
@codecov
Copy link

codecov bot commented Dec 13, 2020

Codecov Report

Merging #444 (58026b4) into master (ddec444) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #444   +/-   ##
=======================================
  Coverage   94.44%   94.44%           
=======================================
  Files         187      187           
  Lines        8244     8244           
=======================================
  Hits         7786     7786           
  Misses        458      458           
Impacted Files Coverage Δ
sdk/test/common/circular_buffer_test.cc 98.97% <0.00%> (-1.03%) ⬇️
sdk/src/logs/batch_log_processor.cc 95.06% <0.00%> (+1.23%) ⬆️

@lalitb
Copy link
Member

lalitb commented Dec 14, 2020

Though I don't have strong opinion, would be good to get other's opinion for the right repo to merge this exporter PR, and other similar PRs - main or contrib. As per spec guidelines - https://github.com/open-telemetry/opentelemetry-specification/blob/0229140bb13e8473f7c51513afd12e2a60618488/specification/library-guidelines.md#requirements
it seems only limited set of exporters can go in main repo:

The SDK implementation should include the following exporters:

Jaeger.
Zipkin.
Prometheus.
OpenTelemetry Protocol (when the protocol is specified and approved).
Standard output (or logging) to use for debugging and testing as well as an input for the various log proxy tools.
In-memory (mock) exporter that accumulates telemetry data in the local memory and allows to inspect it (useful for e.g. unit tests).

Note: some of these support multiple protocols (e.g. gRPC, Thrift, etc). The exact list of protocols to implement in the exporters is TBD.

Other vendor-specific exporters (exporters that implement vendor protocols) should not be included in language libraries and should be placed elsewhere (the exact approach for storing and maintaining vendor-specific exporters will be defined in the future).

@lalitb
Copy link
Member

lalitb commented Dec 14, 2020

Also, please create a github issue for having an example on how to run this against elastic server. As we have for otlp:
https://github.com/open-telemetry/opentelemetry-cpp/tree/master/examples/otlp

@MarkSeufert MarkSeufert force-pushed the logs-elasticsearch-pr branch from 79e6591 to e1c34b1 Compare December 15, 2020 04:16
@MarkSeufert MarkSeufert force-pushed the logs-elasticsearch-pr branch from e1c34b1 to c2ef1af Compare December 15, 2020 04:29
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM: If we can invest some time for example code as done for OTLP exporter, that would be great:
https://github.com/open-telemetry/opentelemetry-cpp/tree/master/examples/otlp

Or else please create a github issue for same.

@MarkSeufert
Copy link
Contributor Author

I have an Elasticsearch example written in my team repo right now, but I'm holding off for PRing it until this one gets merged. I think Its the same situation for the ostream exporter too, so 2 more future PRs!

"-DEFAULTLIB:crypt32.lib",
"-DEFAULTLIB:Normaliz.lib",
],
"//conditions:default": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self: Really want to clean this up... glad it's working though!

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

AFAIk you only have two concurrency things happening here:

  1. The HTTP libraries connection handling thread(s).
  2. Your exporter, waiting to fire off a batch of data.

The threading code looks a little suspect (especially instantiating a local mutex). Effectively you'll want some kind of memory barrier around access the shared variables between threads or you can use atomic variables to control that.

exporters/elasticsearch/src/es_log_exporter.cc Outdated Show resolved Hide resolved
@open-telemetry open-telemetry deleted a comment from MarkSeufert Dec 18, 2020
@kxyr kxyr force-pushed the logs-elasticsearch-pr branch 7 times, most recently from f7dda89 to 6fd1289 Compare December 22, 2020 10:19
@kxyr kxyr force-pushed the logs-elasticsearch-pr branch from 056e9cc to 0526050 Compare December 22, 2020 10:23
@lalitb lalitb merged commit f462e41 into open-telemetry:master Dec 22, 2020
@lalitb lalitb mentioned this pull request May 31, 2021
3 tasks
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.

5 participants