Skip to content

Conversation

@zarinatlupova
Copy link
Contributor

No description provided.

@Gazizonoki Gazizonoki linked an issue Jun 3, 2025 that may be closed by this pull request
Copy link
Collaborator

@Gazizonoki Gazizonoki left a comment

Choose a reason for hiding this comment

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

  1. Нет разметки спанами
  2. Где-то отсутствует реализация
  3. В CMake не вписана зависимость на opentelemetry библиотеку
  4. Надо сделать CMake флаг, чтобы явно включать opentelemetry плагин. Если пользователю он не нужен, не надо делать зависимость кода от него, на то это и плагин

)

_ydb_sdk_install_targets(TARGETS nayuki_md5)
_ydb_sdk_install_targets(TARGETS nayuki_md5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лишние изменения

Copy link
Collaborator

Choose a reason for hiding this comment

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

Отступы в CMakeLists везде по 2 пробела

set(YDB_SDK_GOOGLE_COMMON_PROTOS_TARGET "" CACHE STRING "Name of cmake target preparing google common proto library")
option(YDB_SDK_USE_RAPID_JSON "Search for rapid json library in system" ON)

option(YDB_SDK_TRACING "Enable tracing support" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Флаг не совсем корректно назван, мы включаем не трейсинг целиком, мы включаем именно конкретный opentelemetry плагин

else()
target_link_libraries(tracing_example PRIVATE ydb-cpp-sdk)
target_compile_definitions(tracing_example PRIVATE -DYDB_SDK_TRACING_DISABLED)
endif() No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Таргета ydb-cpp-sdk нет
  2. Более чистым будет линковаться c opentelemetry-cpp так:
target_link_libraries(tracing_example PRIVATE opentelemetry-cpp::api)
  1. Если opentelemetry вырублен, example в принципе собирать не надо

Copy link
Collaborator

Choose a reason for hiding this comment

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

Нужен CMakeLists

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Почему это header файлы?
  2. otel_tracer не нужно компилировать, если проставлен флаг
  3. А как эти трейсеры создать теперь? Надо создать папку plugins в include и src по правильному. Там будет папка для otel_tracer и в нем будет фабричный метод. А то сейчас все в кучу слеплено

@Gazizonoki
Copy link
Collaborator

Все еще нет разметки спанами

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.

Support tracing in SDK and add loading to OpenTelemetry client

2 participants