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

Address spec compliance issue (SetName method is deprecated and removed) #183

Merged

Conversation

maxgolov
Copy link
Contributor

SetName method is now removed in the mainline base class, based on Log spec update here:
open-telemetry/opentelemetry-cpp#1383
open-telemetry/opentelemetry-cpp@280f546

This causes the contrib fluentd build break when the code is compiled with latest OpenTelemetry SDK!

List of changes:

  • Code change : remove override attribute in fluentd log/recordable.h . But keep the method for now. Need to figure out what to do with it in the long run. One possibility is to shim the method to SetAttribute("name", value) in a way that won't be affecting the code / customers that might still be using it.

  • Code change : resolve compilation warnings in examples if HAVE_CONSOLE_LOG has been defined as a build option in the upper level makefile.

  • NEW - non-invasive build infra option : allow to build fluentd exporter inside the main build tree via add_directory. Main idea here is borrowed from nlohmann-json project CMakeLists.txt. If fluentd exporter is plugged into the build via add_directory into main OpenTelemetry C++ SDK, then don't really need to post a separate package of it. And don't need to provide own config. As a result - artifacts get deployed as part of the main project install, in one build loop rather than in two build loops. Much faster. The other option (clone and build as main project) still there, without any functional changes to its behavior. I'll supply a separate README.md for that "fast deployment" later. That's what we use in our Docker image builds / CI pipeline elsewhere.

@maxgolov maxgolov requested review from a team, ThomsonTan, lalitb and owent June 30, 2022 20:17
@lalitb
Copy link
Member

lalitb commented Jun 30, 2022

Thanks for the PR. While we are doing this, should we also update the otel-cpp version used internally (if not already installed) here -

set(opentelemetry-cpp-tag "75c2a8f7a6bf81d9799e76804473609cc236b7be") #to incorporate PR#1325

Else, the build may fail.

@maxgolov
Copy link
Contributor Author

Else, the build may fail.

Done. Updated to latest.

@maxgolov maxgolov merged commit 9fba217 into open-telemetry:main Jun 30, 2022
@marcalff marcalff added the exporter:fluentd Fluentd Exporter label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter:fluentd Fluentd Exporter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants