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.
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-v2] Add support for Elasticsearch #5152
[jaeger-v2] Add support for Elasticsearch #5152
Changes from 16 commits
0afdfd5
c60bf5a
67707ed
a47bb35
5ea1df1
70edeeb
430a0ce
d8d4939
da38c6a
001e4a0
f93b1c6
9aa2c26
c5c20ce
bcdf8a0
236067c
72c5c42
79130b4
e373c3d
e646a3f
e61cf78
6824ec4
999d336
66f63b5
a86760c
7ad3b0d
8dc35cb
0744562
275786c
3763aa9
495daaf
0f7d2f1
a6cec26
915e83f
278d49a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I wonder if there's a test we could write for ensuring that these annotations are correct (btw, what is "dive"?)
We could do it brute-force and just write individual tests for individual fields, but it seems redundant as that would be already done in the validator package (hopefully). But at the same time, having no tests to even ensure that the syntax of the annotations is correct is troublesome.
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.
Comment for L58 and below - can you try removing
yaml:...
annotations? I think they are left-overs from earlier attempts to build on OTEL Coll, I don't think we're using them anywhere.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.
removed it
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.
dive instructs the validation library to examine each element within the slice individually
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.
we can write unit tests that check whether the validation behaves as expected. Instead of testing individual fields, we can create a set of test cases that cover various scenarios, including both valid and invalid configurations.
should i create one?
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.
but that wouldn't be what I am after, such test would simply verify that the Validator is working as expected, on select fields. But if I declare a field with annotations that don't make sense, I want to catch that in unit tests. E.g. misspell
dive
asdiev
(btw weird option, "recursive" would've been much more self explanatory).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.
We can use the "github.com/go-playground/validator" library in our unit tests to detect misspelled annotations and fields with invalid annotation combinations.
like this
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.
what about "github.com/asaskevich/govalidator", will it do the same? We do not use "github.com/go-playground/validator"
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.
Nope, it doesn't do the same. I actually tried using the existing validator at first.
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.
I wonder, if only Primary and not Archive is ever initialized, would it actually provide an archive storage to the UI? Did you try it out?
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.
yes, you are right it dose not work with ui. it throw's this error
docker container used to test it
do you know what causes it?🤔 , i tried fixing it with different configuration of
Options
but couldn't fix.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.
Better to look in the server logs.
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.
Can you put together a docker-compose that illustrates this issue?
This "all shards failed" error has always annoyed me, I wonder if there are more details in the error returned from ES that we're not logging that could be more descriptive. Like, it's ridiculous for a database to respond with "your query failed" instead of telling why (the fact that it failed on "all shards" is not an explanation of why).
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.
sure, i'll make one.
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.
I didn't check this one previously because there were no errors in the UI. However, I've just ran it with elasticsearch and found this issue.
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.
when i checked with
badger
andmemstore
strangely{"archiveStorage":false}
in both of themThere 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.
At least for memstore you need to add archive to the config explicitly:
But maybe this is ok - the idea in v2 was to decouple primary and archive storage altogether, so that one could use different types of backends for those.
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.
So in theory the config you have should've worked, because you did define archive storage, but something isn't working.
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.
added a small fix for above problem, which sets
{"archiveEnabled":true}