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.

Sign up for GitHub

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

Jump to bottom

Zipkin exporter add support for v1 api json format #1411

Merged
merged 104 commits into from
Feb 17, 2021
Merged

Zipkin exporter add support for v1 api json format #1411

merged 104 commits into from
Feb 17, 2021

Conversation

robwknox
Copy link
Contributor

@robwknox robwknox commented Nov 23, 2020 •
edited
Loading

Description

Adds support for the Zipkin Exporter v1 API json format . Since this brings the number of supported formats to 3 (v1-json, v2-json, v2-protobuf) and with v1-thrift on the way, the PR includes notable refactoring of the exporter to introduce the concept of an Encoder.

Breaking changes

  • ZipkinSpanExporter() signature changes:
    • service_name removed to conform to new spec
    • protocol mandatory arg added (required arg ordering change)
    • url => endpoint to be consistent with spec terminology and related env var OTEL_EXPORTER_ZIPKIN_ENDPOINT and package DEFAULT_ENDPOINT
    • ipv4 => local_node_ipv4 to more clearly denote this is for the local node endpoint
    • ipv6 => local_node_ipv6 to more clearly denote this is for the local node endpoint
    • local_node_port: added. This value was being improperly parsed from the url when it is supposed to represent the local node listening port. Added param so that it can be passed in along with the local node ip addresses if desired.
    • retry removed for now to avoid confusion since the logic is not currently implemented.

Fixes #1173

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Manual testing against local zipkin server:
    • v1-json support
    • v1-json / v2-json / v2-protobuf output comparison testing
    • regression testing of v2-json and v2-protobuf
  • Unit tests have been greatly reworked & expanded.
  • Full tox run

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@robwknox
Zipkin exporter v2 api support for protobuf format
a756db0
@robwknox
adding env var OTEL_EXPORTER_ZIPKIN_TRANSPORT_FORMAT and tidying up T…
0197866 
…RANSPORT_FORMAT_JSON/PROTOBUF vals to not overload w/ Content-Type
@robwknox
lint corrections
131cbea
@robwknox
moving auto-gen'd files to separate dir in order to add exclusion ent…
bd7eef7 
…ry in pyproject.toml for black
@robwknox
flake8 config update to exclude new auto-gen files
a8a2407
@robwknox
pylint hints to help with auto-gen protobuf objects
f9c4ff8
@robwknox
Merge branch 'master' into 1175_zipkin_exporter_v2_protobuf_support
d74518d
@robwknox
documentation update
8ce0dc9
@robwknox
switching exporter to use OTEL Configuration class instead of direct …
b6d7e6b 
…os.environ access for retrieving env vars
@robwknox
refactor of SPAN_KIND map structure and loosening of allowed protobuf…
17bf5f7 
… pkg version
@robwknox
pylint disable directive
980b028
@robwknox
docstring format correction
c6ce2a4
@robwknox
Merge branch 'master' into 1175_zipkin_exporter_v2_protobuf_support
8cb1c60
@robwknox
Merge branch 'master' into 1175_zipkin_exporter_v2_protobuf_support
1bcc834
@robwknox
Create enum class to represent supported transport formats
c9889f7
@robwknox
Refactoring of zipkin exporter code to add abstraction to allow for e…
f94129d 
…asier addition of supported API formats
@robwknox
v1 json api transport format support
16e1463
@robwknox
formatting corrections
fc66495
@robwknox
Major refactor (TODO: update commit msg)
ff2dc50
@robwknox
correction to test logic to handle nested unordered dicts in python 3…
9b9b9d7 
….5 + a bug fix related to max_tag_value_length in the encoders
@robwknox
unit test refactoring and additions + new abstract methods encode_tra…
934cee0 
…ce_id() and encode_span_id() and base Encoder class
@robwknox
Bug fix: os env override should only occur in ZipkinSpanExporter and …
3bc9840 
…not in the Sender
@robwknox
removing unnecesseray error logging
afa5173
@robwknox
additional tests for exporter constructor
0f498bf
@robwknox
Merge branch 'master' into 1175_zipkin_exporter_v2_protobuf_support
bb2fec4
@robwknox
black
8dc15d7
@robwknox
lint
8cdf3af
@robwknox
Merge branch 'master' into 1173_zipkin_exporter_v1_json_support
8d0277e
@robwknox
Merge branch '1173_zipkin_exporter_v1_json_support' into 1173_zipkin_…
789b1a3 
…exporter_v1_json_support_refactor
@robwknox
New os env var OTEL_EXPORTER_ZIPKIN_SERVICE_NAME
8a511f4
Base automatically changed from master to main January 29, 2021 16:49
ocelotl
ocelotl suggested changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Just nits, @robwknox would it be possible to address @lonewolf3739 comments?

exporter/opentelemetry-exporter-zipkin/tests/encoder/test_v1_json.py Show resolved Hide resolved
srikanthccv
srikanthccv approved these changes Feb 10, 2021
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. LGTM.

exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/encoder/__init__.py Outdated Show resolved Hide resolved
@robwknox robwknox mentioned this pull request Feb 10, 2021
Closed
@robwknox
Copy link
Contributor Author

@lonewolf3739 , @ocelotl , @codeboten believe all comments are addressed.

codeboten
codeboten approved these changes Feb 13, 2021
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a minor change in the comment about the env variable. Solid refactor, thanks a lot!

exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/encoder/__init__.py Outdated Show resolved Hide resolved
@codeboten
Copy link
Contributor

@ocelotl has your change request been addressed? If so, please approve, thanks!

@ocelotl
Copy link
Contributor

ocelotl commented Feb 17, 2021

Sure, will review

@codeboten codeboten merged commit 03c6b73 into open-telemetry:main Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv approved these changes

@ocelotl ocelotl ocelotl approved these changes

@codeboten codeboten codeboten approved these changes

@aabmass aabmass Awaiting requested review from aabmass

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Zipkin exporter must support Zipkin v1 JSON format
5 participants
@robwknox @codeboten @ocelotl @srikanthccv @lzchen