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

Remove opentelemetry-exporter-otlp-proto-common dependency #40

Conversation

sfc-gh-jopel
Copy link
Contributor

@sfc-gh-jopel sfc-gh-jopel commented Oct 30, 2024

This PR

  • Internalizes some encoding logic from opentelemetry-exporter-otlp-proto-common under the directory src/snowflake/internal/opentelemetry/exporter/**
  • Modifies the above logic to use the custom serialization structures introduced in Add code generator for otel proto #39
  • Updates tests to check for equality of serialized bytes rather than PB2 objects, since the new code only produces bytes
  • Adds a micro-benchmark to compare encoding performance of the original library and the new approach under the benchmarks/** directory

Todo in future PRs:

  • Add import time and memory benchmarks
  • Add branching / try..except import to use the original opentelemetry-exporter-otlp-proto-common if it is present in the runtime instead of the custom logic
  • Add a step to the release script & Jenkins job to test the .conda / .tar.bz2 package in a conda environment end to end
  • Release v0.6.0
------------------------------------------------------------------------------
Benchmark                                    Time             CPU   Iterations
------------------------------------------------------------------------------
test_bm_serialize_logs_data_4MB      218404625 ns    218404667 ns            3
test_bm_pb2_serialize_logs_data_4MB  279917764 ns    279916000 ns            3
test_bm_serialize_logs_data              35747 ns        35747 ns        19741
test_bm_pb2_serialize_logs_data          41652 ns        41651 ns        16939
test_bm_serialize_metrics_data           59798 ns        59798 ns        11593
test_bm_pb2_serialize_metrics_data       70521 ns        70521 ns         9815
test_bm_serialize_traces_data            47156 ns        47156 ns        14807
test_bm_pb2_serialize_traces_data        59690 ns        59690 ns        11766

https://docs.google.com/document/d/1plfwWBVRJGzU5dl3kph4HWwgnxBc8TJm0JCyanCdJuM/edit?usp=sharing

@@ -0,0 +1,110 @@
# Copyright The OpenTelemetry Authors
Copy link
Collaborator

@sfc-gh-sili sfc-gh-sili Oct 30, 2024

Choose a reason for hiding this comment

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

@sfc-gh-bdrutu
Do you have any advice how we should update this copyright?
Is it okay to simply change it to Snowflake since Apache Liscense (https://en.wikipedia.org/wiki/Apache_License) "allows users to use, modify, and distribute software for any purpose"?

With a note like
"Adapted from github.com/opentelemetry/opentelemetry-proto/xxxxx/init.py"

return LogsData(
resource_logs=encode_logs(batch).resource_logs # pylint: disable=no-member
).SerializeToString()
return encode_logs(batch)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename this API since we do different thing from what opentelemetry is doing.

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.

2 participants