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

Internalize opentelemetry-exporter-otlp-proto-common adapter code #43

Merged

Conversation

sfc-gh-jopel
Copy link
Contributor

@sfc-gh-jopel sfc-gh-jopel commented Nov 21, 2024

Description

  • This PR internalizes opentelemetry-exporter-otlp-proto-common encoder / adapter logic that converts SDK objects into serializable pb2 protobuf generated objects
  • Adds a script vendor_otlp_proto_common.sh that clones the repo for a desired version, and replaces the necessary imports in the file. The copied files retain the copyright notice and append a notice that the file has been modified
    • Adds workflow to ensure generated code matches latest from script
    • Adds unit test to ensure the version of vendored in code matches the installed sdk version
  • The import of pb2 objects is replaced to point at custom marshaler objects that provide the same interface and serialize the same bytestrings as protobuf objects, to remove the protobuf dependency
    • Removes the runtime dependency on opentelemetry-exporter-otlp-proto-common, and protobuf by extension
  • Diff between internalized code and original code from upstream repo: https://gist.github.com/sfc-gh-jopel/b99a506acb43ebf5ef0f9ffefd87bc7c
    • only diff is the imports of opentelemetry.proto objects, all other logic is the same
      git diff --no-index ~/opentelemetry-python/exporter/opentelemetry-exporter-otlp-proto-common/src/opentelemetry/exporter/otlp/proto/common ~/snowflake-telemetry-python/src/snowflake/telemetry/_internal/opentelemetry/exporter/otlp/proto/common

Upcoming PRs

Benchmarks

  • Benchmark added to compare opentelemetry-exporter-otlp-proto-common encoder using protobuf vs internalized opentelemetry-exporter-otlp-proto-common encoder using pure python marshaler objects.
    • For _log_encoder: custom is <3% slower than protobuf
    • For metrics_encoder: custom is ~35% faster than protobuf
    • For trace_encoder: custom is ~10% faster than protobuf
  • It seems that metrics_encoder has a big performance improvement because the encoder code involves instantiating many small pb2 objects, and the overhead of instantiating C native extension objects is magnified in this case. _log_encoder is on the other end of the spectrum where there are minimal objects instantiated and the structure is more flat, so the overhead is not magnified and instead this code path sees more of the benefit of faster serialization times after the shorter adapter logic is executed.
------------------------------------------------------------------------------
Benchmark                                    Time             CPU   Iterations
------------------------------------------------------------------------------
test_bm_serialize_logs_data_4MB      730591536 ns    730562298 ns            1
test_bm_pb2_serialize_logs_data_4MB  702522039 ns    702490893 ns            1
test_bm_serialize_logs_data             100882 ns       100878 ns         6930
test_bm_pb2_serialize_logs_data          97112 ns        97109 ns         7195
test_bm_serialize_metrics_data          114938 ns       114934 ns         6096
test_bm_pb2_serialize_metrics_data      161849 ns       161845 ns         4324
test_bm_serialize_traces_data           123977 ns       123973 ns         5633
test_bm_pb2_serialize_traces_data       131016 ns       131011 ns         5314

Testing

  • Modifies tests to compare equality of bytestrings instead of pb2 objects
    • Tests still have a dependency on opentlemetry-proto to ensure there is no regression

@sfc-gh-jopel sfc-gh-jopel requested a review from a team as a code owner November 21, 2024 17:11
@sfc-gh-sili
Copy link
Collaborator

Looks good! Thank you, Jeevan!

@sfc-gh-jopel sfc-gh-jopel force-pushed the internalize-update-encoder-code branch from c87e194 to 27ae3c9 Compare December 10, 2024 18:56
Copy link
Collaborator

@sfc-gh-tmonk sfc-gh-tmonk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sfc-gh-jopel sfc-gh-jopel merged commit 196413d into snowflakedb:main Dec 11, 2024
7 checks passed
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.

3 participants