Add Elasticsearch Log Exporter with tests #18

MarkSeufert · 2020-12-11T21:54:24Z

This PR adds an Elasticsearch exporter for logs, which is a requirement from issue #337. This includes:

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

Some overlap exists with the OStream exporter PR (open-telemetry#430) currently, but will be addressed in a future PR.

Notes:

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.

cc @xukaren @alolita

exporters/elasticsearch/src/es_log_exporter.cc

codecov-io · 2020-12-11T23:14:05Z

Codecov Report

Merging #18 (056e9cc) into master (e4d0b2c) will increase coverage by 0.07%.
The diff coverage is 97.12%.

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   94.38%   94.45%   +0.07%     
==========================================
  Files         185      187       +2     
  Lines        8014     8244     +230     
==========================================
+ Hits         7564     7787     +223     
- Misses        450      457       +7

Impacted Files	Coverage Δ
.../include/opentelemetry/common/key_value_iterable.h	`100.00% <ø> (ø)`
api/include/opentelemetry/nostd/function_ref.h	`77.77% <ø> (ø)`
api/include/opentelemetry/nostd/shared_ptr.h	`100.00% <ø> (ø)`
api/include/opentelemetry/nostd/span.h	`89.36% <ø> (+1.36%)`	⬆️
api/include/opentelemetry/nostd/string_view.h	`97.50% <ø> (ø)`
api/include/opentelemetry/nostd/unique_ptr.h	`100.00% <ø> (ø)`
api/include/opentelemetry/nostd/utility.h	`83.33% <ø> (ø)`
api/include/opentelemetry/trace/span.h	`100.00% <ø> (ø)`
api/test/logs/provider_test.cc	`100.00% <ø> (ø)`
api/test/nostd/string_view_test.cc	`100.00% <ø> (ø)`
... and 24 more

alolita

Missing copyright header: None of the source files have the OpenTelemetry authors copyright. Please make sure all source files filed as PRs have this copyright header.
Add header comments noting purpose of each source file
Add inline function headers noting purpose of each function

kxyr · 2020-12-15T04:44:10Z

exporters/elasticsearch/test/es_log_exporter_test.cc

+  record->SetAttribute("key2", "value2");
+
+  // Write the log record to the exporter, and time the duration
+  auto t1     = std::chrono::high_resolution_clock::now();


would std::chrono::system_clock::now() suffice?

kxyr · 2020-12-15T04:46:17Z

exporters/elasticsearch/src/es_log_exporter.cc

+{}
+
+std::unique_ptr<sdklogs::Recordable> ElasticsearchLogExporter::MakeRecordable() noexcept
+{


nit: if the exporter is shut down, does it still need to make a recordable? ie. could it return nullptr?

Saw your discussion on ostream, leaving as is

kxyr · 2020-12-15T04:47:42Z

exporters/elasticsearch/src/es_log_exporter.cc

+  request->SetUri(options_.index_ + "/_bulk?pretty");
+  request->SetMethod(http_client::Method::Post);
+  request->AddHeader("Content-Type", "application/json");
+  request->SetTimeoutMs(std::chrono::milliseconds(1000 * options_.response_timeout_));


nit: could just use microseconds instead of multiplying by 1000?

I was using seconds because http timeouts can be fairly large (like 30 seconds), so typing 30 makes more sense than 30000 imo.

I mean would request->SetTimeoutMs(std::chrono::microseconds(options_.response_timeout_)); work?
Edit: ignore this, woul dneed std::duration_cast

kxyr · 2020-12-15T05:16:36Z

exporters/elasticsearch/src/es_log_exporter.cc

+        static_cast<ElasticSearchRecordable *>(record.release()));
+    body += json_record->GetJSON().dump() + "\n";
+  }
+  std::vector<uint8_t> body_vec(body.begin(), body.end());


question: why is body set as a uint8_t?

in Lalit's http client interface the body is represented internally as a vector of uint8_t, so I want to keep consistent with that

kxyr · 2020-12-15T05:17:30Z

exporters/elasticsearch/src/es_log_exporter.cc

+   */
+  std::string GetResponseBody()
+  {
+    if (!response_received_)


nit: is it possible to check if response_received before entering this function instead? not sure if setting a random response body here is good practice or

yea it's being checked in the export method already, I had this as a just in case kinda thing tho. I'll take it out tho since it's never gonna get called

kxyr · 2020-12-15T05:17:50Z

exporters/elasticsearch/src/es_log_exporter.cc

+  bool response_received_ = false;
+
+  // A string to store the response body
+  std::string body_ = "";


i don't think this needs a default initialization? the default string ctor should be fine :)

kxyr · 2020-12-15T05:19:02Z

exporters/elasticsearch/src/es_log_exporter.cc

+  request->SetTimeoutMs(std::chrono::milliseconds(1000 * options_.response_timeout_));
+
+  // Add the request body
+  std::string body = "";


is this variable not already initialized as a private member variable? (see comment above) does this overwrite it?

nope, the body member variable is for the response classes response body, and this is for the request body

ohh i see, could they maybe be named like response_body and request_body? idk if you think thats necessary

kxyr · 2020-12-15T05:20:23Z

exporters/elasticsearch/test/es_log_exporter_test.cc

+{
+  // Create options for the elasticsearch exporter
+  logs_exporter::ElasticsearchExporterOptions options("localhost", -1, "logs", 5, true);
+  options.response_timeout_ = 10;  // Wait 10 seconds to receive a response


is this overkill? (if the CI needed to wait 10 seconds while running this test, I'm just wondering if this will be a problem)

Considering Lalit's http unit test does this except with 30 seconds, I think its fine :)

Ohh I see yeah, I get wait during the CI and I'm always afraid it's deadlocking or sth, gotcha gotcha

kxyr · 2020-12-15T05:21:44Z

exporters/elasticsearch/test/es_log_exporter_test.cc

+  auto exporter =
+      std::unique_ptr<sdklogs::LogExporter>(new logs_exporter::ElasticsearchLogExporter);
+  bool shutdownResult = exporter->Shutdown();
+  ASSERT_TRUE(shutdownResult);


nit: EXPECT_TRUE(exporter->Shutdown());?

kxyr · 2020-12-15T05:24:42Z

exporters/elasticsearch/test/es_log_exporter_test.cc

+
+  // Ensure the timeout is within the range of the timeout specified ([10, 10 + 1] seconds)
+  auto duration = std::chrono::duration_cast<std::chrono::seconds>(t2 - t1).count();
+  ASSERT_TRUE((duration >= options.response_timeout_) &&


nit: i'm confused why the timeout here has such a strict lowerbound? (i thought anything greater than 0 would be acceptable) - is it sufficient to just check ASSERT_TRUE(duration < options.response_timeout_ +1)?

Here I'm checking that it waits for the timeout instead of immediately returning failure. Its important for this exporter to wait for at least the timeout to give time for a connection/response to come.

kxyr · 2020-12-15T05:26:48Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h

+   * @param host The host of the Elasticsearch instance
+   * @param port The port of the Elasticsearch instance
+   * @param index The index/shard that the logs will be written to
+   * @param response_timeout The maximum time the exporter should wait after sending a request to


nit: specify whether this timeout is in milliseconds, microseconds, etc?

kxyr · 2020-12-15T05:27:14Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h

+  ElasticsearchExporterOptions(std::string host     = "localhost",
+                               int port             = 9200,
+                               std::string index    = "logs",
+                               int response_timeout = 30,


related nit: would it be better just to use std::chrono::milliseconds/microseconds here isntead of int?

Also curious- what's the difference/purpose of passing in the params separately into the constructor, as opposed to just the plain old constructor with the options struct?

kxyr · 2020-12-15T05:28:15Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h

+  /**
+   * Exports a vector of log records to the Elasticsearch instance. Guaranteed to return after a
+   * timeout specified from the options passed from the constructor.
+   * @param records A list of log records to send to Elasticsearch.


nit: because it might be confusing log records with the LogRecord recordable: change comment to say just a list of "logs"?

kxyr · 2020-12-15T05:32:37Z

exporters/elasticsearch/src/es_log_exporter.cc

+  // Return failure if this exporter has been shutdown
+  if (isShutdown_)
+  {
+    if (options_.console_debug_)


nit: add {} around this if block too?

kxyr · 2020-12-15T05:33:04Z

exporters/elasticsearch/src/es_log_exporter.cc

+  {
+    // TODO: retry logic
+
+    if (options_.console_debug_)


{} here (i forget which place i commented on in the upstream PR :p)

kxyr · 2020-12-15T05:40:27Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h

+   * localhost:9200/logs with a timeout of 30 seconds and disabled console debugging
+   * @param host The host of the Elasticsearch instance
+   * @param port The port of the Elasticsearch instance
+   * @param index The index/shard that the logs will be written to


just wondering - should this index variable also be used in the body_ variable? which currently has the default index of index:{}

the way elasticsearch works is you connect to it with an endpoint, like localhost::3000/logs for example. The /logs part is the index, so having the index:{} means that it will use the index specified in the endpoint instead of a specific one.

kxyr · 2020-12-15T05:41:53Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h

+
+private:
+  // Stores if this exporter had its Shutdown() method called
+  bool isShutdown_ = false;


another nit: is_shutdown_ for google naming convention

I'm also just wondering through (relates to the ostream exporter too) - do you think this var should be initialized in the ctor instead of here?

hmm having it here means we don't have to set it in all the different constructors (this one has 2), but I'm also not sure about if OTEL encourages this... I'll ask during the next maintainers meeting!

kxyr · 2020-12-15T05:42:35Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h

+   * @param index The index/shard that the logs will be written to
+   * @param response_timeout The maximum time the exporter should wait after sending a request to
+   * Elasticsearch
+   * @param console_debug Print the status of the exporter methods in the console


nit: rename var to console_debug_on to indicte it's a bool? and maybe the comment could be: "whether or not this exporter prints [...]", so it indicates this is a flag to be toggled

To me, if there's a bool variable called console_debug its pretty explicit that turning it on is going to enable console debugging. So I think leaving it as is is cool

kxyr · 2020-12-15T05:46:32Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_recordable.h

+  /**
+   * Returns a JSON object contain the log information
+   */
+  nlohmann::json GetJSON() noexcept { return json_; };


nit: GetJSONRecord()?

kxyr · 2020-12-15T05:49:35Z

Sorry for all the comments - "nit" has also become my favorite word :)

MarkSeufert force-pushed the logs-elasticsearch-pr branch from 2da8cda to 4255ac1 Compare December 11, 2020 22:18

kxyr reviewed Dec 11, 2020

View reviewed changes

exporters/elasticsearch/src/es_log_exporter.cc Show resolved Hide resolved

alolita changed the title ~~Add Elasticsearch Log Exporter + Tests~~ Add Elasticsearch Log Exporter with tests Dec 11, 2020

kxyr force-pushed the master branch 2 times, most recently from 0d650af to 68e510c Compare December 12, 2020 03:34

alolita reviewed Dec 12, 2020

View reviewed changes

MarkSeufert force-pushed the logs-elasticsearch-pr branch 3 times, most recently from 79e6591 to e1c34b1 Compare December 15, 2020 04:16

MarkSeufert added 10 commits December 14, 2020 21:21

Elasticsearch exporter with basic tests

0752ecd

Removed local dependancy on nlohmann:json

405cc81

Storing attributes/resource in JSON, helper methods

9996974

Mock server partial completion

aa1485c

Formatting and remove cmake build

31fe565

formatting

0033b6d

thread sanatization fix, formatting

22f1266

Fixed Windows Bazel build issue

faa8de5

Made sessionmanager a private member

b358555

Add JSON Recordable

c2ef1af

MarkSeufert force-pushed the logs-elasticsearch-pr branch from e1c34b1 to c2ef1af Compare December 15, 2020 04:29

kxyr reviewed Dec 15, 2020

View reviewed changes

MarkSeufert added 8 commits December 15, 2020 10:24

JSON attributes, other nits

8b6404d

CMakeList and nit

b48638d

Added OnEvent() callback implementation

c8b4225

Removed unneeded using from header

1e735b9

Recordable attributes and resource duplicate code put into function

3b65136

Merge branch 'master' into logs-elasticsearch-pr

8a5b8fb

Added mutex as private member

0f57f83

formatting

f05aead

kxyr force-pushed the logs-elasticsearch-pr branch 7 times, most recently from f7dda89 to 6fd1289 Compare December 22, 2020 10:19

Use updated attribute utils to fix failing bazel CI

0526050

kxyr force-pushed the logs-elasticsearch-pr branch from 056e9cc to 0526050 Compare December 22, 2020 10:23

Merge branch 'master' into logs-elasticsearch-pr

58026b4

MarkSeufert closed this Dec 22, 2020

Add Elasticsearch Log Exporter with tests #18

Add Elasticsearch Log Exporter with tests #18

Conversation

MarkSeufert commented Dec 11, 2020 • edited Loading

codecov-io commented Dec 11, 2020 • edited Loading

Codecov Report

alolita left a comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxyr commented Dec 15, 2020

MarkSeufert commented Dec 11, 2020 •

edited

Loading

codecov-io commented Dec 11, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading

kxyr Dec 15, 2020 •

edited

Loading