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

Add Zipkin support of OpenSearch storage #3765

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

reta
Copy link
Contributor

@reta reta commented Apr 17, 2024

Add Zipkin support of OpenSearch storage

The Elasticsearch storage now supports OpenSearch as a backed as well. The implementation relies on distribution property that is returned as part of Elasticsearch / OpenSearch HTTP GET / API. Please note that the storage version is now abstracted as BaseVersion with two implementation: ElasticsearchVersion and OpensearchVersion.

Although OpenSearch is a fork of Elasticsearch as of 7.10.2, the projects diverged sufficiently far from each other. Luckily, Zipkin relies on the features that have not been impacted (so far) and work the same way across both projects.

  • Add javadocs
  • Add documentation

@reta reta force-pushed the issue-3753 branch 3 times, most recently from 77197e1 to 7679e99 Compare April 17, 2024 23:33
@codefromthecrypt
Copy link
Member

I think it is easiest to make the image first in a separate PR. Then, merge and we can use it.

I don't recall the history of the feature ask, but if someone hasn't asked for 1.x maybe better to only do 2.x because there's a linear time increase per images. Usually we don't add old versions as a first storage option.

At any rate, you would do a PR for the image, and make sure it is in the docker readme test workflows etc. When that merges, after master builds clean, you can re-cut the last version docker images as docker-x.x.x. Finally, as it is a new image, it will be private by default, so you need to click to make it public https://github.com/orgs/openzipkin/packages

codefromthecrypt pushed a commit that referenced this pull request Apr 30, 2024
Add Zipkin support of OpenSearch storage (add Docker images) as dicussed
#3765 (comment)

---------

Signed-off-by: Andriy Redko <drreta@gmail.com>
@codefromthecrypt
Copy link
Member

@reta for the error around ghcr.io/openzipkin/zipkin-opensearch2:3.3.1 you want to push a tag docker-3.3.1 off latest master. this will re-publish and also include the new one, unless we missed a spot somewhere.

@codefromthecrypt
Copy link
Member

@reta
Copy link
Contributor Author

reta commented May 1, 2024

@reta for the error around ghcr.io/openzipkin/zipkin-opensearch2:3.3.1 you want to push a tag docker-3.3.1 off latest master. this will re-publish and also include the new one, unless we missed a spot somewhere.

Thanks @codefromthecrypt , it seems like we need to make a 3.3.1 release first since Docker images are looking for Zipkin 3.3.1 bits ...

@codefromthecrypt
Copy link
Member

@reta oh sorry.. I didn't remember what release was current. change the test code to use 3.3.0 and re-cut docker-3.3.0. that's the simplest way and will get the new test image

@reta
Copy link
Contributor Author

reta commented May 2, 2024

also don't forget to set this public https://github.com/orgs/openzipkin/packages/container/zipkin-opensearch2/settings

@codefromthecrypt apologies, I see the image is 100% pushed but I cannot see it in the Packages, is it my permissions? (because it seems like I cannot change visibility level for existing packages as well) I see that I could change the visibility for existing packages

@codefromthecrypt
Copy link
Member

switched visibility to public ;) https://github.com/orgs/openzipkin/packages/container/zipkin-opensearch2/settings

@codefromthecrypt
Copy link
Member

when ready, re-prep the PR description, after making sure any markdown that need a usage example update have been so. Also, anything in general READMEs about things we test mention opensearch since after this, it is true.

Signed-off-by: Andriy Redko <drreta@gmail.com>
Signed-off-by: Andriy Redko <drreta@gmail.com>
@codefromthecrypt
Copy link
Member

also bump poms 3.3.1-SNAPSHOT -> 3.4.0-SNAPSHOT as def not a patch ;)

Signed-off-by: Andriy Redko <drreta@gmail.com>
@codefromthecrypt
Copy link
Member

also I think a question originally was about hosted aws opensearch (zipkin-aws). want to try integrating with that via snapshot locally prior to mark this for review?

@reta
Copy link
Contributor Author

reta commented May 3, 2024

also I think a question originally was about hosted aws opensearch (zipkin-aws). want to try integrating with that via snapshot locally prior to mark this for review?

Absolutely, thanks @codefromthecrypt !

@reta
Copy link
Contributor Author

reta commented May 8, 2024

also I think a question originally was about hosted aws opensearch (zipkin-aws). want to try integrating with that via snapshot locally prior to mark this for review?

@codefromthecrypt I have to admit failure to verify the flow, I was not able to get AWS managed OpenSearch instance(s) for testing purposes. On an optimistic note, AFAIK the managed OpenSearch uses the same open source distribution and should work seamlessly.

@codefromthecrypt
Copy link
Member

okie dokie. I'm a bit swamped, but if I've not reviewed this by saturday, nag me as I should be able to get time to look closely by then.

Copy link
Member

@codefromthecrypt codefromthecrypt 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 doing this! As far as I understand, this makes a heuristic internally to detect OS vs ES, so that the server config pretty much remains the same. Do you know if zipkin-dependencies works with OS using the current driver? Because merging this without dependencies working will immediately lead to another problem.

p.s. maybe cc anyone who has asked for OS on this PR so that they can know about it directly

@@ -3,7 +3,7 @@
This is a plugin to the Elasticsearch storage component, which uses
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the detailed updates here

}

@Nested
class ITEnsureIndexTemplate extends zipkin2.elasticsearch.integration.ITEnsureIndexTemplate {
Copy link
Member

Choose a reason for hiding this comment

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

I forget.. this appears duplicated and is using the method elasticsearch() not the field. Can this be pulled up?

Copy link
Contributor Author

@reta reta May 9, 2024

Choose a reason for hiding this comment

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

Sorry, you mean replace elasticsearch() -> elasticsearch? (or better to say opensearch in this case)

@reta
Copy link
Contributor Author

reta commented May 9, 2024

Do you know if zipkin-dependencies works with OS using the current driver?

Ah, thanks a lot for pointing it out @codefromthecrypt, it won't work with Elasticsearch, we will need to add OpenSearch module alongside the Elasticsearch one.

…onTest

Signed-off-by: Andriy Redko <drreta@gmail.com>
@reta
Copy link
Contributor Author

reta commented May 9, 2024

Hm ... lint check failure is new:

 build-bin/configure_lint: 5: markdown-link-check: not found

added 66 packages in 3s

16 packages are looking for funding
  run `npm fund` for details
usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA] [--list-files]
                [-f {parsable,standard,colored,github,auto}] [-s]
                [--no-warnings] [-v]
                [FILE_OR_DIR ...]
yamllint: error: one of the arguments FILE_OR_DIR - is required

@codefromthecrypt
Copy link
Member

let's hold merging this until that's sorted out as I don't want to have a new storage option that doesn't work due to zipkin-dependencies (bomb)

… ElasticsearchSpecificTemplatesTest

Signed-off-by: Andriy Redko <drreta@gmail.com>
@reta
Copy link
Contributor Author

reta commented May 9, 2024

let's hold merging this until that's sorted out as I don't want to have a new storage option that doesn't work due to zipkin-dependencies (bomb)

I could pick it up later this week, very likely over the weekend (the good thing, AFAIK OpenSearch has Spark / Hadoop support on par with Elasticsearch)

@codefromthecrypt
Copy link
Member

thanks @reta!

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.

None yet

2 participants