-
Notifications
You must be signed in to change notification settings - Fork 525
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
Jaeger http thrift #3081
Merged
Merged
Jaeger http thrift #3081
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## jaeger #3081 +/- ##
==========================================
+ Coverage 79.27% 79.45% +0.18%
==========================================
Files 101 103 +2
Lines 5279 5365 +86
==========================================
+ Hits 4185 4263 +78
- Misses 1094 1102 +8
|
simitt
requested changes
Dec 19, 2019
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.
Code looks great!
Only found some typo, for which it is concerning that data are properly decoded with the typo.
The certificates weren't being exercised in the tests, and when I changed the tests to use them the CA wouldn't verify the certificate. Regenerated using mkcert.
Shuffle existing Jaeger config around a little bit to make room for HTTP config.
Add an HTTP handler/muxer, for accepting Thrift-encoded trace data over HTTP. Refactor GRPCServer into Server, which encapsulates both gRPC and HTTP servers. Move beater/api/jaeger code into beater/jaeger, which is the only user of GRPCCollector. Simplify tests to use a mock TraceConsumer, rather than sending through the concrete processor/otel.Consumer. Fix TLS-related tests to actually use TLS, and add tests that prove it's enabled.
And add approvals for Thrift-over-HTTP.
axw
force-pushed
the
jaeger-http-thrift
branch
from
December 20, 2019 02:13
5d21139
to
39debaf
Compare
simitt
approved these changes
Dec 20, 2019
axw
added a commit
that referenced
this pull request
Jan 9, 2020
* Integrate Jaeger gRPC collector (#2976) Add support for Jaeger gRPC Trace Intake Collector. The gRPC endpoint collects monitoring metrics and supports TLS communication, by reusing the `apm-server.ssl.*` configuration. By default the gRPC endpoint is disabled. closes #2962 Co-Authored-By: Andrew Wilkins <[email protected]> * [Jaeger] Add otel consumer converting batches to Elastic APM events (#3066) Add consumer converting incoming otel batches to Elastic APM format. Add integration tests covering incoming gRPC requests being transformed to beat events. partially implements #3307 * Jaeger http thrift (#3081) Add an HTTP handler, muxer, and server, in beater/jaeger for accepting Thrift-encoded trace data over HTTP. Refactor beater/jaeger.GRPCServer into Server, which now encapsulates both gRPC and HTTP servers. Move beater/api/jaeger code into beater/jaeger, which is the only user of GRPCCollector. If the beater/jaeger code grows significantly, we might consider having subpackages like beater/jaeger/grpc, beater/jaeger/http, etc. * [jaeger] Convert Timeevents to errors (#3085) * [jaeger] Convert Timeevents to errors Parse Timeevents from Jaeger spans and convert to elastic error events if they describe an error. Fixes #3007 * Add experimental flag to Jaeger integration (#3121) * tests/system: system test for Jaeger Thrift/HTTP (#3114) * tests/system: system test for Jaeger Thrift/HTTP * tests/system: system test for Jaeger gRPC * processor/otel: update approvals Co-authored-by: Silvia Mitter <[email protected]>
simitt
pushed a commit
to simitt/apm-server
that referenced
this pull request
Jan 14, 2020
Add an HTTP handler, muxer, and server, in beater/jaeger for accepting Thrift-encoded trace data over HTTP. Refactor beater/jaeger.GRPCServer into Server, which now encapsulates both gRPC and HTTP servers. Move beater/api/jaeger code into beater/jaeger, which is the only user of GRPCCollector. If the beater/jaeger code grows significantly, we might consider having subpackages like beater/jaeger/grpc, beater/jaeger/http, etc.
simitt
pushed a commit
to simitt/apm-server
that referenced
this pull request
Jan 14, 2020
Add an HTTP handler, muxer, and server, in beater/jaeger for accepting Thrift-encoded trace data over HTTP. Refactor beater/jaeger.GRPCServer into Server, which now encapsulates both gRPC and HTTP servers. Move beater/api/jaeger code into beater/jaeger, which is the only user of GRPCCollector. If the beater/jaeger code grows significantly, we might consider having subpackages like beater/jaeger/grpc, beater/jaeger/http, etc.
simitt
added a commit
that referenced
this pull request
Jan 14, 2020
* Integrate Jaeger gRPC collector (#2976) Add support for Jaeger gRPC Trace Intake Collector. The gRPC endpoint collects monitoring metrics and supports TLS communication, by reusing the `apm-server.ssl.*` configuration. By default the gRPC endpoint is disabled. closes #2962 Co-Authored-By: Andrew Wilkins <[email protected]> * [Jaeger] Add otel consumer converting batches to Elastic APM events (#3066) Add consumer converting incoming otel batches to Elastic APM format. Add integration tests covering incoming gRPC requests being transformed to beat events. partially implements #3307 * Jaeger http thrift (#3081) Add an HTTP handler, muxer, and server, in beater/jaeger for accepting Thrift-encoded trace data over HTTP. Refactor beater/jaeger.GRPCServer into Server, which now encapsulates both gRPC and HTTP servers. Move beater/api/jaeger code into beater/jaeger, which is the only user of GRPCCollector. If the beater/jaeger code grows significantly, we might consider having subpackages like beater/jaeger/grpc, beater/jaeger/http, etc. * [jaeger] Convert Timeevents to errors (#3085) * [jaeger] Convert Timeevents to errors Parse Timeevents from Jaeger spans and convert to elastic error events if they describe an error. Fixes #3007 * Add experimental flag to Jaeger integration (#3121) * tests/system: system test for Jaeger Thrift/HTTP (#3114) * tests/system: system test for Jaeger Thrift/HTTP * tests/system: system test for Jaeger gRPC Co-authored-by: Andrew Wilkins <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add an HTTP handler, muxer, and server, in
beater/jaeger
for accepting Thrift-encoded trace data over HTTP. Refactorbeater/jaeger.GRPCServer
intoServer
, which now encapsulates both gRPC and HTTP servers.Move
beater/api/jaeger
code into beater/jaeger, which is the only user of GRPCCollector. If thebeater/jaeger
code grows significantly, we might consider having subpackages likebeater/jaeger/grpc
,beater/jaeger/http
, etc.Config has been updated to accommodate both gRPC and HTTP. Config now looks like this:
Tests have been simplified to use mock TraceConsumers, rather than relying on the concrete processor/otel.Consumer. They're integrating less now; we can test the whole process in system tests.
I regenerated the TLS certificates under testdata, as the CA certificate did not appear to be related to the certificate. TLS wasn't being enabled correctly before - that's been fixed now, with tests added that prove it's enabled.
Closes #3008