-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add code generator for otel proto #39
Add code generator for otel proto #39
Conversation
Thanks for the work! |
compile/plugin.py
Outdated
@@ -0,0 +1,237 @@ | |||
#!/usr/bin/env python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit hard to understand without context. Can you add some explanation about this plug in and the standard protoc generator in the PR description? Is it simply a slimmed down version of protoc or does it do anything special?
tests/test_compile.py
Outdated
message AnyValue { | ||
oneof value { | ||
string string_value = 1; | ||
bool bool_value = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be easier if this is a seperate file called, say, tests/data/simple.proto
compile/plugin.py
Outdated
@@ -0,0 +1,237 @@ | |||
#!/usr/bin/env python3 | |||
|
|||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense to have this file (and template) under scripts
since it is only used when we generate from proto.
Unless that stops tests from reading this file.
In general, I am convinced that this is a solid approach. We just want to make sure we have enough doc/comment to understand this a year from now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks
Keeping a note here for future reference: Before these gets into a new release, we need a test that validates this serialization against the official version. Psudo-code:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I like the use of the custom plugin to generate the pure Python serialization code we need.
Could we create a Jenkins job that regularly tests the full build flow, now that it is getting more complex?
- code generation
- build the package (.tar.bz2 and .conda)
- running tests against the built packages, including, like Sindy mentioned a comparison against serialized messages generated by official Python protobuf packages?
ccb9852
to
b42e003
Compare
b42e003
to
6f519c4
Compare
This reverts commit 0fd842e.
This reverts commit 0fd842e.
This is the first of a few PRs to add custom marshaling logic in place of opentelemetry-exporter-otlp-proto-common. The objective is to remove the hard runtime dependency on protobuf from snowflake-telemetry-python. This first PR
More detailed explanation:
src/snowflake/telemetry/_internal/opentelemetry/proto/*
is the code generated by running./scripts/proto_codegen.sh
.scripts/plugin.py
is a plugin for the protoc compiler. It uses protoc to generate marshallng code for the custom message types defined in opentelemetry-proto. The responsibility of this generated code is to produce the same serialized bytestrings as pb2 generated structure would, given the same inputs. Additionally, handwritten util code located insrc/snowflake/telemetry/_internal/serialize
and is used to serialize primitive protobuf types, like varint, string, bytes, ints, etc.