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

[EXPORTER] Refactor ElasticSearchRecordable #3164

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sjinks
Copy link
Contributor

@sjinks sjinks commented Nov 22, 2024

Changes

  • Simplify ElasticSearchRecordable::WriteValue() by using the ADL Serializer;
  • Simplify ElasticSearchRecordable::SetTimestamp();
  • Refactor ElasticSearchRecordable::SetTraceId() and ElasticSearchRecordable::SetSpanId() by using constants for array sizes instead of hardcoded values.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added (in another PR)
  • Changes in public API reviewed

Copy link

netlify bot commented Nov 22, 2024

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit b72c41b
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/6744171d3f2e7100087c5e2d

{
static void to_json(json &j, const opentelemetry::sdk::common::OwnedAttributeValue &v)
{
opentelemetry::nostd::visit([&j](const auto &value) { j = value; }, v);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A possible breaking change here is that the original WriteValue() did not handle spans. If this is important, I can add an if condition to follow the old behavior.

Comment on lines -243 to +128
char trace_buf[32];
char trace_buf[opentelemetry::trace::TraceId::kSize * 2];
Copy link
Contributor Author

@sjinks sjinks Nov 22, 2024

Choose a reason for hiding this comment

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

This approach is safer: should kSize change someday, the buffer's size will be automatically updated.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.86%. Comparing base (31956f8) to head (b72c41b).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3164   +/-   ##
=======================================
  Coverage   87.86%   87.86%           
=======================================
  Files         195      195           
  Lines        6151     6151           
=======================================
  Hits         5404     5404           
  Misses        747      747           
---- 🚨 Try these New Features:

@sjinks sjinks marked this pull request as ready for review November 23, 2024 01:42
@sjinks sjinks requested a review from a team as a code owner November 23, 2024 01:42

std::strcat(bufferDate, bufferMilliseconds);
std::strcat(bufferDate, "Z");
std::ostringstream oss;
Copy link
Member

Choose a reason for hiding this comment

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

Why change it to put_time and ostringstream? In my understandarding, using strftime and snprintf with just one static buffer block will have better performence than using ostringstream and put_time with dynamic memory allocation. And there are also some compatiblity problems with put_time and old compiler's STL.

Copy link
Contributor Author

@sjinks sjinks Nov 25, 2024

Choose a reason for hiding this comment

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

A newer version of gcc complained:

warning: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Wformat-truncation=]
   47 |     std::snprintf(bufferMilliseconds, sizeof(bufferMilliseconds), ".%06ld",
      |                                                                          ^
In function ‘int snprintf(char*, size_t, const char*, ...)’,
/usr/include/x86_64-linux-gnu/bits/stdio2.h:54:35: note: ‘__builtin___snprintf_chk’ output between 8 and 9 bytes into a destination of size 8
   54 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   55 |                                    __glibc_objsize (__s), __fmt,
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   56 |                                    __va_arg_pack ());
      |                                    ~~~~~~~~~~~~~~~~~

so I tried to come up with something safe that does not involve hardcoded sizes, compiles cleanly, does not have a potential for buffer overflows, and is as fast as std::format("{:%FT%T%Ez}", timePoint).

For example, in

  const static int dateToSecondsSize = 19;
  const static int millisecondsSize  = 8;

it was not immediately evident that millisecondsSize reserves one byte for the null terminator (.123456 is seven bytes, not eight), and it felt like dateSize = dateToSecondsSize + millisecondsSize + timeZoneSize; char bufferDate[dateSize]; could lead to a buffer overflow.

Anyway, I think I have found a faster solution not involving strcat and hardcoded buffer sizes.

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.

2 participants