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

OTLP/GRPC Exporter Implementation #272

Merged
merged 37 commits into from
May 5, 2021

Conversation

SeanHood
Copy link
Contributor

@SeanHood SeanHood commented Mar 4, 2021

What's working?

  • Sends spans to the Otel Collector
  • Spans contain: Attributes, Status Code, Kind, Start/End timestamp, Lib Version
  • Successfully sent OTLP to Honeycomb.io

What's not working?

  • Need to rebase main onto this branch
  • Trace ID and Span ID are too long, and have been fudged to trim them.
  • Some tests. Mostly for SpanConverter, lacking in tests for Exporter
  • Events on spans haven't been tested
  • Child spans haven't been tested
  • Example code needs to be updated (been testing with laravel-otel)
  • Need to check the status returned
  • Generally just unfinished

Anything more to add?

  • Thank you to the Ruby OTLP Exporter, lots of inspiration was taken from there
  • Thanks to @riticksingh for starting off the OTLP/GRPC work

riticksingh and others added 16 commits November 12, 2020 00:28
updated script file
updated Makefile
What's working?
* Sends spans to the Otel Collector
* Spans contain: Attributes, Status Code, Kind, Start/End timestamp, Lib Version
* Sucessfully sent OTLP to Honeycomb.io

What's not working?
* Need to rebase main onto this branch
* Trace ID and Span ID are too long, and have been fudged to trim them.
* No tests. Unit/Integ
* Events on spans haven't been tested
* Child spans haven't been tested
* Example code needs to be updated (been testing with laravel-otel)
* Need to check the status returned
* Links not implemented
* Generally just unfinished

Anything more to add?
* Thank you to the Ruby OTLP Exporter, lots of inspiration was taken from there
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 4, 2021

CLA Signed

The committers are authorized under a signed CLA.

@bobstrecansky
Copy link
Collaborator

@zsistla - I know you were planning on working on something similar, wanted to tag you for awareness.

contrib/OtlpGrpc/Exporter.php Show resolved Hide resolved
contrib/OtlpGrpc/Exporter.php Show resolved Hide resolved
contrib/OtlpGrpc/Exporter.php Outdated Show resolved Hide resolved
@zsistla
Copy link
Contributor

zsistla commented Mar 5, 2021 via email

$this->compression = getenv('OTEL_EXPORTER_OTLP_COMPRESSION') ?: $compression; // TODO
$this->timeout =(int) getenv('OTEL_EXPORTER_OTLP_TIMEOUT') ?: $timeout; // TODO

$this->spanConverter = new SpanConverter('foo');
Copy link
Contributor

Choose a reason for hiding this comment

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

Other exporters have passed the service name (foo in this case) in via the constructer but it can also be set from OTEL_RESOURCE_ATTRIBUTES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've looked into this a little more, I see that most of the PHP Exporters take in the serviceName. So I went down the rabbit hole of OTEL_RESOURCE_ATTRIBUTES in other languages, and they seem to pass in service.name etc into a Resource on the TracerProvider rather than passing the service name via the Exporter. See these two examples:

https://github.com/lightstep/opentelemetry-examples/blob/main/go/opentelemetry/server/server.go#L116
https://github.com/honeycombio/examples/blob/main/python-otlp/app.py#L20

Which is swaying me more to removing the $serviceName from the Exporter entirely. What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

If OTEL_RESOURCE_ATTRIBUTES exists it is prioritized; if it doesn't exist, then the service.name is set in the tracer and should be pulled from there to set the service name for export.

You can get the ResourceInfo from the tracer then iterate through the keys and set the attributes on the span like python.
https://github.com/open-telemetry/opentelemetry-python/blob/main/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py#L127

@zsistla
Copy link
Contributor

zsistla commented Mar 21, 2021

The progress you made on this is fantastic! Once you make the metadata not hardcoded, it'll be completely flexible to send to whatever backend(s) the collector is set up for. If you comment out those lines, the make collector-trace will automatically send it.

Actually, I don't think the metadata doesn't even needs to be passed here. You can set it in the collector configuration file. For example, the config file for honeycomb would be something like this.

Great job :)

@bobstrecansky bobstrecansky mentioned this pull request Mar 23, 2021

$sdk->setSpanStatus(SpanStatus::OK);

$sdk->end($end_time); // Setting the end timestamp does not work how I expect it to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the line that I'm struggling to make the test pass. /cc @bobstrecansky

  1. Tried passing in a timestamp
  2. Tried not passing a timestamp, but fetching it later with $sdk->getEnd() and passing this to the Proto Span didn't work either.

I've ran into complications with setting an end time/duration when trying to write database instrumentation in Laravel too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I spoke with Sean about this in DM, I think we can use:

Clock::get())->timestamp()

To accomplish his timing demands here.

@SeanHood
Copy link
Contributor Author

SeanHood commented Apr 9, 2021

I asked on one of the weekly calls, but I forgot to add a comment here.

I think we need to generate the proto files and add them into the repo. In the call it seemed like a good idea.

  1. One less thing end users will need to do
  2. Will help get tests to pass for this PR (without generating them just for the tests)

@bobstrecansky
Copy link
Collaborator

Definitely happy to add the proto files to the repo!

SeanHood added 3 commits April 9, 2021 23:29
I pushed the proto files and build is complaining, not sure if we want to exclude this dir from linting or not?

Testing it formatted so I can try to get this build green.
1. Moved out ResourcesSpans into the SpanConverter
2. Time is almost fixed, well tests pass but... I learned the clock didn't work as I expected it to. Wall clock and monotonic clock are entirely unrelated. I still think this needs more work but I _think_ it tests that the duration is correctly set on the otlp span.
3. ResourceInfo is now populated on otlp ResourceSpans.
@hrodic
Copy link

hrodic commented Apr 14, 2021

@SeanHood SeanHood changed the title (WIP): Partially working OTLP/GRPC Exporter OTLP/GRPC Exporter Implementation May 5, 2021
@SeanHood SeanHood marked this pull request as ready for review May 5, 2021 16:02
@SeanHood
Copy link
Contributor Author

SeanHood commented May 5, 2021

Hey all,

Flagging this as ready for review. There's a few things which still need to be fixed but I believe it's 90% there.

Since it's almost there, merging it will open the floor for more contributors to resolve some of these without being blocked.

@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #272 (f68a7f1) into main (8bc993e) will decrease coverage by 0.00%.
The diff coverage is 92.10%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #272      +/-   ##
============================================
- Coverage     94.09%   94.09%   -0.01%     
- Complexity      495      551      +56     
============================================
  Files            47       49       +2     
  Lines          1185     1337     +152     
============================================
+ Hits           1115     1258     +143     
- Misses           70       79       +9     
Impacted Files Coverage Δ Complexity Δ
contrib/OtlpGrpc/Exporter.php 92.06% <92.06%> (ø) 25.00 <25.00> (?)
contrib/OtlpGrpc/SpanConverter.php 92.13% <92.13%> (ø) 31.00 <31.00> (?)
sdk/Trace/Span.php 93.26% <0.00%> (+2.88%) 42.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bc993e...f68a7f1. Read the comment docs.

Copy link
Collaborator

@bobstrecansky bobstrecansky left a comment

Choose a reason for hiding this comment

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

The codecov part of this is unimportant to me right now. I think we can merge this.

Copy link
Contributor

@zsistla zsistla left a comment

Choose a reason for hiding this comment

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

Great job on all your work on this!!!

@bobstrecansky bobstrecansky merged commit 8a13501 into open-telemetry:main May 5, 2021
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.

5 participants