dd-trace-go icon indicating copy to clipboard operation
dd-trace-go copied to clipboard

Published 20 hours ago • verified publisher icon DataDog

contrib/elastic/go-elasticsearch: Add support for elasticsearch v8

Open xrn opened this issue 2 years ago • 8 comments

  • Removed v6 from directory name
  • Deleted tests for Elasticsearch v6
  • Added support for v8

xrn avatar Jun 29 '22 09:06 xrn

@mrkm4ntr as you worked with https://github.com/DataDog/dd-trace-go/pull/1017 - do you have maybe idea why this test is failing?

xrn avatar Jun 29 '22 11:06 xrn

@gbbr what do you think? My goal is when ES8 will be merged I want to add support for OpenSearch 2.0 https://github.com/opensearch-project/OpenSearch

xrn avatar Jul 05 '22 12:07 xrn

@gbbr Issue added

xrn avatar Jul 05 '22 13:07 xrn

@gbbr question: Do you know maybe why pull of image https://www.docker.elastic.co/r/elasticsearch/elasticsearch:8.2.0 is working but there are problems with pull https://www.docker.elastic.co/r/elasticsearch/elasticsearch:8.3.0? Should I upgrade something more?

xrn avatar Jul 31 '22 18:07 xrn

Hello! Thanks for putting up a PR for this, instead of dropping the v6 tests I think we should leave them and just add the v8 tests as well. (Since we will continue to provide tracing for v6 although it's worth us including a comment that it's a deprecated version). I'll go through a add some additional comments as well :)

ajgajg1134 avatar Aug 01 '22 15:08 ajgajg1134

@ajgajg1134 just do confirm with you that we are on the same page.

  • In last PR of this type - https://github.com/DataDog/dd-trace-go/pull/1017 - you removed v5 supprt
  • For Golang you are making legacy versions as well https://github.com/DataDog/dd-trace-go/pull/1410

But here we would like to leave v6 even deprecated version and I should just add v8?

xrn avatar Aug 11 '22 06:08 xrn

@xrn

In last PR of this type - https://github.com/DataDog/dd-trace-go/pull/1017 - you removed v5 supprt

The PR description is misleading. We didn’t remove v5 support, we just didn't added new tests for v5 at the time of that PR, since this was the first time we were adding support for elastic search. The PR was saying that it wasn't going to add new tests for something that was already legacy when the support was being introduced.

For Golang you are making legacy versions as well https://github.com/DataDog/dd-trace-go/pull/1410

Supporting old versions of the language is very different from supporting old versions of a library. We already have a documented policy around language version support. But there’s no need to break backwards compatibility just because something is deprecated, at least not without a migration plan.

But here we would like to leave v6 even deprecated version and I should just add v8?

Yes. There's no need to delete tests and possibly break backwards compatibility at this time.

katiehockman avatar Sep 02 '22 19:09 katiehockman

@katiehockman thanks for response in next week I will update PR

xrn avatar Sep 03 '22 18:09 xrn

@katiehockman made some changes, I think it should be fine now. Not sure why but Kafka test are failing, even if I do not introduced any changes.

FYI I would be in a trip next 2 weeks if you would like to merge earlier feel free to take over the PR

xrn avatar Oct 14 '22 11:10 xrn

Hm, I wanted to reopen this PR to get this contribution merged, but I don't think I can actually push changes since the fork was deleted.

I will continue in a follow-up PR.

VJean avatar Feb 23 '23 11:02 VJean