-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
WIP: Elasticsearch uses Span2 internally #1651
Conversation
84f22b7
to
60d7885
Compare
60d7885
to
a352881
Compare
added a "shared" field to help with status quo where libraries on the client and server side usually share a span ID. #1499 (comment) Made one of the converters, but lacking tests at the moment. Once there's 2-way conversion, I'll open another PR trying out ES or similar |
a352881
to
423f3a3
Compare
lack of tests was driving me bonkers. Backfilled tests so that next change (which includes conversion code) is more sane |
423f3a3
to
c91a5cd
Compare
c91a5cd
to
4e7ff56
Compare
I've a working span converter, but still needs a lot more tests as there are many ways data can come in (and come in unexpectedly) |
to move this forward, I'm first temporarily converting the internals of InMemorySpanStore to use SimpleSpan (and the converter). This will help break tests and show bugs before trying in elasticsearch. |
e6f8d8f
to
5b0d520
Compare
ok have almost all storage tests working.. should finish it tomorrow and move to ES |
5b0d520
to
1d47a0b
Compare
update: all spanstore tests work, but some dependency linker tests don't probably missing some edge cases |
1d47a0b
to
e07c6c6
Compare
ok almost there on the data conversion. a couple more tests to go |
edf10fa
to
a1937ec
Compare
all tests are good with local span store. I moved onto @openzipkin/elasticsearch, but ran into a glitch and need feedback on the idea or reverting to span.timestamp/duration (so that duration query is effortless) #1499 (comment) |
a1937ec
to
dc72958
Compare
tentatively changed from startTimestamp/finishTimestamp to timestamp/duration to see if elasticsearch becomes easier (almost certainly will) |
@openzipkin/elasticsearch I added a commit for elasticsearch which uses the new format internally (no change to the format collectors use). Can I get a guinea pig to test this out by building this branch and throwing traffic at it? Bear in mind it requires ES 2.4+ |
e32a28e
to
2b28827
Compare
FYI working on dependency link now. deferring ES as I expect that it might be effortful for folks to test a branch |
dependency linking (killing DependencyLinkSpan for SimpleSpan) went well enough except one-way spans. I might have to re-jig the converters a bit to keep span store tests passing and also pass on dependency links. Glad I hit a snag as this stuff really needs to be solid. |
2b28827
to
e147e1f
Compare
cleanups all done and provably we won't need DependencyLinkSpan after this. We can use the same span type for input, storage, query and aggregation. To that end, I've raised a comment whether to rename SimpleSpan to zipkin2.Span. Will proceed in whichever direction following a couple people feeding back. Regardless, it will be chopping this PR into smaller pieces. |
What's the timeline? First changes to internal storage then API or all together? |
Here's a thought about order
First is adding the type SimpleSpan or v2 span whatever we decide and
converters.
Then add the json serialisation code
In any order we add content type handler for post, kafka etc transports
Also in any order we add ElasticSearch support (storage format). We can add
this as optional flag especially as indexing needs to be looked at
carefully.
All of the above can be done pretty quickly once we make a decision on
naming, but I would probably aim for elasticsearch even before transports
because there are a lot of wins there.
…On 27 Jul 2017 5:53 am, "Jordi Polo Carres" ***@***.***> wrote:
What's the timeline? First changes to internal storage then API or all
together?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1651 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD617t5ul8HK71Svx5IWNWZmMTyuqtkks5sR7U8gaJpZM4OVQvn>
.
|
So everything mentioned including ES internal in next release is a
proposal. Next minor from that would improve or formalize that and add
transport (http kafka) support.
…On 27 Jul 2017 8:42 am, "Adrian Cole" ***@***.***> wrote:
Here's a thought about order
First is adding the type SimpleSpan or v2 span whatever we decide and
converters.
Then add the json serialisation code
In any order we add content type handler for post, kafka etc transports
Also in any order we add ElasticSearch support (storage format). We can
add this as optional flag especially as indexing needs to be looked at
carefully.
All of the above can be done pretty quickly once we make a decision on
naming, but I would probably aim for elasticsearch even before transports
because there are a lot of wins there.
On 27 Jul 2017 5:53 am, "Jordi Polo Carres" ***@***.***>
wrote:
> What's the timeline? First changes to internal storage then API or all
> together?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#1651 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAD617t5ul8HK71Svx5IWNWZmMTyuqtkks5sR7U8gaJpZM4OVQvn>
> .
>
|
The flag is a good idea, would allow for incremental adoption. I would imagine that we just wipe out old data and store new in the new format. I think most users do not store the data for long anyways |
@jcarres-mdsol I think I found a way to tease out a lot of the code safely. If I start with swapping the internal DependencyLinkSpan for an internal copy of what's now SimpleSpan, we can get that code out there and make sure it works. Later will be a simple import change to formalize the type. |
first thing splitting off is the span model type (no codec or converters) #1669 |
This adds an internal copy of the new span data structure defined in issue 1499. This is starting internal to ease review and allow incremental progress. The first consumer will be dependency linking.
This adds an internal copy of a span conversion utility mentioned in issue #1499. This is starting internal to ease review and allow incremental progress. The first consumer will be dependency linking.
This adds an internal copy of a span json codec issue #1499. This starts internal to ease review and allow incremental progress. The first consumer will be Elasticsearch, as this format removes nested queries. Note: this change also introduces json serialization of Span2, which allows future use in Spark.
e147e1f
to
e0e07ee
Compare
This drops the internal type of DependencyLinkSpan in favor of the Span2 type coming in #1499. Doing so now gives us practice, solves a few bugs along the way. When Span2 becomes non-internal, the change will be a simple package rename.
d405350
to
066c6b5
Compare
e0e07ee
to
bc439f0
Compare
All the rejigging PRs are green. I plan to merge the ones leading up to this (Elasticsearch spike) tomorrow. If folks are keen on the new span kinds for producer/consumer, I'll add a PR for that. Once the above is done, I'll look at how hard it is to make this have an internal flag to use the "span2" type conditionally (right now, this branch won't read old data only new data). If it is straightforward enough, I'll push that out with a minor bump adding instructions here on how to enable it. In the mean time, anyone can try this branch on their own. It writes to the same indexes, but using the "span2" type and no "servicespan" index. |
bc439f0
to
8aaf7bb
Compare
587e362
to
d977317
Compare
heh.. something's up as github won't let me re-open this |
carried forward in #1674 |
This uses the span2 model from #1499 to implement Elasticsearch without using nested queries. This requires code from #1673