Skip to content

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

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