Remove deprecated es.max-num-spans #2482

BernardTolosajr · 2020-09-17T02:03:09Z

Which problem is this PR solving?

Remove deprecated --es.max-num-spans flag in v1.21.0 - resolves Remove deprecated --es.max-num-spans flag in v1.21.0 #2457
Fixes part of Remove deprecated CLI flags #2738

Short description of the changes

Remove deprecated ES option --es.max-num-spans

albertteoh · 2020-09-17T03:10:48Z

Thanks for quickly jumping on this, @BernardTolosajr. We're currently on 1.19.2 and we're targeting this for release 1.21.0 to give folks a chance to migrate over to --es.max-doc-count, so I'm not sure if we can merge this change just yet.

I've also restarted Crossdock and Unit Test jobs which appear to be caused by flaky tests.

@BernardTolosajr, I think you'll need to remove the code block here as well: https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/options.go#L295

Basically, grep -ir maxNumSpans and remove accordingly.

codecov · 2020-09-17T03:16:10Z

Codecov Report

Merging #2482 (3d1c18d) into master (27cb88f) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2482      +/-   ##
==========================================
- Coverage   95.88%   95.84%   -0.04%     
==========================================
  Files         217      217              
  Lines        9647     9638       -9     
==========================================
- Hits         9250     9238      -12     
- Misses        328      330       +2     
- Partials       69       70       +1

Impacted Files	Coverage Δ
plugin/storage/es/options.go	`100.00% <ø> (ø)`
cmd/collector/app/server/zipkin.go	`73.07% <0.00%> (-3.85%)`	⬇️
cmd/query/app/static_handler.go	`95.16% <0.00%> (-1.62%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27cb88f...3d1c18d. Read the comment docs.

BernardTolosajr · 2020-09-17T05:45:51Z

Noted on this one.

albertteoh

lgtm

yurishkuro

I am blocking this for now to prevent being merged accidentally before v1.21

albertteoh · 2021-01-22T00:40:37Z

@BernardTolosajr it's a good time to merge this PR now that we've made a couple of releases. Can you please look at resolving the merge conflict? Let me know if you need help with this. Thanks.

yurishkuro · 2021-01-22T01:31:35Z

fwiw, there are LOTS of other deprecated flags (#2738). But we should not clean them in a single PR since they usually require code changes, better to do one PR per flag.

./cmd/flags/admin.go:39:	healthCheckHTTPPortWarning = "(deprecated, will be removed after 2020-03-15 or in release v1.19.0, whichever is later)"
./cmd/flags/admin.go:40:	adminHTTPPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/collector/app/builder_flags.go:45:	collectorHTTPPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/collector/app/builder_flags.go:46:	collectorGRPCPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/collector/app/builder_flags.go:47:	collectorZipkinHTTPPortWarning = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/agent/app/reporter/flags.go:51:		flags.String(AgentTagsDeprecated, "", "(deprecated) see --"+agentTags)
./cmd/query/app/flags.go:42:	queryPortWarning        = "(deprecated, will be removed after 2020-08-31 or in release v1.20.0, whichever is later)"
./cmd/query/app/flags.go:43:	queryHOSTPortWarning    = "(deprecated, will be removed after 2020-12-31 or in release v1.21.0, whichever is later)"
./plugin/storage/cassandra/options.go:237:		"(deprecated) Jaeger will automatically detect the version of the dependencies table")
./plugin/storage/cassandra/options.go:241:		"(deprecated) Enable (or disable) host key verification. Use "+nsConfig.namespace+".tls.skip-host-verify instead")
./plugin/storage/es/options.go:186:		"(deprecated, will be removed in release v1.21.0. Please use "+nsConfig.namespace+".max-doc-count). "+
./pkg/config/tlscfg/flags.go:54:		flags.Bool(c.Prefix+tlsEnabledOld, false, "(deprecated) see --"+c.Prefix+tlsEnabled)
./pkg/config/tlscfg/flags.go:69:		flags.Bool(c.Prefix+tlsEnabledOld, false, "(deprecated) see --"+c.Prefix+tlsEnabled)
./pkg/config/tlscfg/flags.go:74:	flags.String(c.Prefix+tlsClientCAOld, "", "(deprecated) see --"+c.Prefix+tlsClientCA)

Pull request has been modified.

BernardTolosajr · 2021-01-30T06:33:30Z

@albertteoh pushed fixed for the merge conflict. Let me know if i miss something. Thanks

BernardTolosajr · 2021-01-30T07:35:42Z

@albertteoh should i also remove test for MaxNumSpansUsage?
https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/options_test.go#L103

Thanks

albertteoh · 2021-01-30T10:23:37Z

@albertteoh should i also remove test for MaxNumSpansUsage?
https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/options_test.go#L103

Yes, please @BernardTolosajr.

yurishkuro

please add an entry to changelog as a "breaking change"

Signed-off-by: Bernard Tolosa <bernardotolosajr@gmail.com>

Signed-off-by: Bernardo Tolosa <bernardotolosajr@gmail.com>

yurishkuro · 2021-02-01T01:51:31Z

CHANGELOG.md


-#### New Features
+### New Features


the original level 4 is correct

yurishkuro · 2021-02-01T01:53:03Z

CHANGELOG.md

@@ -7,8 +7,9 @@ Changes by Version
 ### Backend Changes

 #### Breaking changes
+* Remove deprecated flags `es.max-num-spans` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))


Suggested change

* Remove deprecated flags `es.max-num-spans` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))

* Remove deprecated flag `--es.max-num-spans`, please use `--es.max-doc-count` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))

Pull request has been modified.

Signed-off-by: Bernardo Tolosa <bernardotolosajr@gmail.com>

yurishkuro

@BernardTolosajr Thank you!

BernardTolosajr requested a review from a team as a code owner September 17, 2020 02:03

BernardTolosajr requested a review from pavolloffay September 17, 2020 02:03

BernardTolosajr force-pushed the remove-es-max-num-spans branch from 3c81b2f to 0932061 Compare September 18, 2020 00:57

albertteoh approved these changes Sep 18, 2020

View reviewed changes

yurishkuro previously requested changes Sep 18, 2020

View reviewed changes

objectiser mentioned this pull request Nov 12, 2020

1.21 release #2627

Closed

joe-elliott mentioned this pull request Nov 13, 2020

Preparing release 1.21.0 #2630

Merged

BernardTolosajr force-pushed the remove-es-max-num-spans branch from 0932061 to c7999bc Compare January 30, 2021 06:27

yurishkuro reviewed Jan 31, 2021

View reviewed changes

BernardTolosajr force-pushed the remove-es-max-num-spans branch from 7959a5d to 7b7f29d Compare February 1, 2021 01:30

BernardTolosajr added 2 commits February 1, 2021 09:34

Remove deprecated es.max-num-spans

bd5dcc1

Signed-off-by: Bernard Tolosa <bernardotolosajr@gmail.com>

Removed maxNumSpansUsage test

d0b060b

Signed-off-by: Bernardo Tolosa <bernardotolosajr@gmail.com>

BernardTolosajr force-pushed the remove-es-max-num-spans branch 2 times, most recently from fd09be5 to 900c550 Compare February 1, 2021 01:50

yurishkuro previously approved these changes Feb 1, 2021

View reviewed changes

BernardTolosajr force-pushed the remove-es-max-num-spans branch from 900c550 to 148ec86 Compare February 1, 2021 01:55

Add breaking change entry to CHANGELOG

59e3e6a

Signed-off-by: Bernardo Tolosa <bernardotolosajr@gmail.com>

BernardTolosajr force-pushed the remove-es-max-num-spans branch from 148ec86 to 59e3e6a Compare February 1, 2021 02:01

Merge branch 'master' into remove-es-max-num-spans

3d1c18d

yurishkuro mentioned this pull request Feb 1, 2021

Remove deprecated CLI flags #2738

Closed

7 tasks

yurishkuro approved these changes Feb 1, 2021

View reviewed changes

yurishkuro merged commit 9e5acac into jaegertracing:master Feb 1, 2021

dependabot bot mentioned this pull request Mar 8, 2021

Bump github.com/jaegertracing/jaeger from 1.21.0 to 1.22.0 httpsgithu/opentelemetry-collector#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated es.max-num-spans #2482

Remove deprecated es.max-num-spans #2482

BernardTolosajr commented Sep 17, 2020 •

edited by yurishkuro

Loading

albertteoh commented Sep 17, 2020

codecov bot commented Sep 17, 2020 •

edited

Loading

BernardTolosajr commented Sep 17, 2020

albertteoh left a comment

yurishkuro left a comment

albertteoh commented Jan 22, 2021

yurishkuro commented Jan 22, 2021 •

edited

Loading

BernardTolosajr commented Jan 30, 2021

BernardTolosajr commented Jan 30, 2021

albertteoh commented Jan 30, 2021

yurishkuro left a comment

yurishkuro Feb 1, 2021

yurishkuro Feb 1, 2021

yurishkuro left a comment

	* Remove deprecated flags `es.max-num-spans` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))
	* Remove deprecated flag `--es.max-num-spans`, please use `--es.max-doc-count` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))

Remove deprecated es.max-num-spans #2482

Remove deprecated es.max-num-spans #2482

Conversation

BernardTolosajr commented Sep 17, 2020 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

albertteoh commented Sep 17, 2020

codecov bot commented Sep 17, 2020 • edited Loading

Codecov Report

BernardTolosajr commented Sep 17, 2020

albertteoh left a comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

albertteoh commented Jan 22, 2021

yurishkuro commented Jan 22, 2021 • edited Loading

BernardTolosajr commented Jan 30, 2021

BernardTolosajr commented Jan 30, 2021

albertteoh commented Jan 30, 2021

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Feb 1, 2021

Choose a reason for hiding this comment

yurishkuro Feb 1, 2021

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

BernardTolosajr commented Sep 17, 2020 •

edited by yurishkuro

Loading

codecov bot commented Sep 17, 2020 •

edited

Loading

yurishkuro commented Jan 22, 2021 •

edited

Loading