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] Boolean environment variables not parsed per the spec #1982

Merged
merged 7 commits into from
Feb 17, 2023

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Feb 13, 2023

Fixes #1859 [EXPORTER] Boolean environment variables not parsed per the spec

Changes in SDK

  • Moved the implementation of GetEnvironmentVariable() from .h
    to GetRawEnvironmentVariable() in .cc

  • Changed the prototype to return bool exists,
    to decouple environment variables existence from empty value.

  • Implemented typed helpers to properly parse values:

    • GetBoolEnvironmentVariable(),
    • GetDurationEnvironmentVariable(),
    • GetStringEnvironmentVariable()

Changes in Exporters environment variables

  • Moved GetXXXDefaultYYY() helpers from otlp_environment.h to .cc

  • Renamed all helpers for orthogonality:

    • helpers dedicated to a signal have the signal name, in plural form
    • helpers dedicated to Http or Grpc are named accordingly.
  • This exposed many bugs and inconsistencies, for example for Endpoint()

    • Before:

      • GetOtlpDefaultGrpcEndpoint() -- Misnamed, it is for traces
      • GetOtlpDefaultHttpEndpoint() -- Misnamed, it is for traces
      • GetOtlpDefaultHttpLogEndpoint() -- GrpcLog missing
      • GetOtlpDefaultMetricsEndpoint() -- is it for Http or Grpc ?
    • After:

      • GetOtlpDefaultGrpcTracesEndpoint()
      • GetOtlpDefaultGrpcMetricsEndpoint()
      • GetOtlpDefaultGrpcLogsEndpoint()
      • GetOtlpDefaultHttpTracesEndpoint()
      • GetOtlpDefaultHttpMetricsEndpoint()
      • GetOtlpDefaultHttpLogsEndpoint()
  • This renaming is a potential breaking change for logs (now in plural form),
    which is acceptable because logs are not fully implemented yet.

  • This renaming is a potential breaking change if a user application
    used existing helpers for traces and metrics,
    so original names are also provided for backward compatibility.
    For example:

// Compatibility with OTELCPP 1.8.2
inline std::string GetOtlpDefaultHttpEndpoint()
{
  return GetOtlpDefaultHttpTracesEndpoint();
}
  • Added all the helpers required to parse environment variables
    defined in the spec, for example for traces SSL/TLS options:

    • GetOtlpDefaultTracesSslCertificatePath();
    • GetOtlpDefaultTracesSslCertificateString();
    • GetOtlpDefaultTracesSslClientKeyPath();
    • GetOtlpDefaultTracesSslClientKeyString();
    • GetOtlpDefaultTracesSslClientCertificatePath();
    • GetOtlpDefaultTracesSslClientCertificateString();
  • In otlp_environment.cc, implemented typed helpers
    for environment variables that can have dual names:

    • GetBoolDualEnvVar()
    • GetDurationDualEnvVar()
    • GetStringDualEnvVar()
  • Refactored all the GetXXXDefaultYYY() implementation to use
    these helpers.

  • For special cases like GetOtlpDefaultTracesIsInsecure(),
    provided a migration path from old variables (OTEL_EXPORTER_OTLP_TRACES_SSL_ENABLE)
    to new variables (OTEL_EXPORTER_OTLP_TRACES_INSECURE)

  • Prepared the code for future variables deprecation,
    with #define WARN_DEPRECATED_ENV.

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

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

[EXPORTER] Boolean environment variables not parsed per the spec
@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #1982 (5155946) into main (e47dec5) will increase coverage by 0.18%.
The diff coverage is 98.60%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1982      +/-   ##
==========================================
+ Coverage   87.11%   87.29%   +0.18%     
==========================================
  Files         166      166              
  Lines        4597     4662      +65     
==========================================
+ Hits         4004     4069      +65     
  Misses        593      593              
Impacted Files Coverage Δ
sdk/src/common/env_variables.cc 98.53% <98.53%> (ø)
sdk/src/resource/resource_detector.cc 100.00% <100.00%> (ø)
sdk/src/trace/batch_span_processor.cc 91.48% <0.00%> (+0.78%) ⬆️

@marcalff marcalff marked this pull request as ready for review February 13, 2023 22:04
@marcalff marcalff requested a review from a team February 13, 2023 22:04
@marcalff
Copy link
Member Author

Ready for review.

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

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

LGTM and thanks.

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. Thanks

sdk/src/common/env_variables.cc Show resolved Hide resolved
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.

[EXPORTER] Boolean environment variables not parsed per the spec
4 participants