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

Fix es-archive namespace default values #2865

Merged

Conversation

albertteoh
Copy link
Contributor

Signed-off-by: albertteoh albert.teoh@logz.io

Which problem is this PR solving?

Short description of the changes

  • Sets the default config of "other" namespaces to the primary's default config.

  • As discussed in Fix archive default values reported for ES and Cassandra #2853, the primary default of 72h is misleading to users because an archive fetch should have no "lookback" limit, fetching any traceID that is in archive storage.

    This PR also removes the --es-archive.max-span-age flag to avoid confusing users.

Testing

  • Confirmed --es-archive.max-span-age no longer appears in --help for both query and collector.
  • Confirmed archive defaults are correctly based on primary config defaults for both query and collector. E.g.:
      --es-archive.max-doc-count int                       The maximum document count to return from an Elasticsearch query. This will also apply to aggregations. (default 10000)

Signed-off-by: albertteoh <albert.teoh@logz.io>
@albertteoh albertteoh requested a review from a team as a code owner March 5, 2021 14:39
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #2865 (c582fb7) into master (70e9df7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2865      +/-   ##
==========================================
- Coverage   95.93%   95.91%   -0.02%     
==========================================
  Files         223      223              
  Lines        9675     9679       +4     
==========================================
+ Hits         9282     9284       +2     
- Misses        325      326       +1     
- Partials       68       69       +1     
Impacted Files Coverage Δ
plugin/storage/es/options.go 100.00% <100.00%> (ø)
cmd/collector/app/server/zipkin.go 73.07% <0.00%> (-3.85%) ⬇️
cmd/query/app/server.go 95.62% <0.00%> (-1.46%) ⬇️
plugin/storage/integration/integration.go 77.90% <0.00%> (+0.55%) ⬆️

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 70e9df7...c582fb7. Read the comment docs.

yurishkuro
yurishkuro previously approved these changes Mar 5, 2021
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Might warrant a "breaking change" entry in the changelog

albertteoh and others added 2 commits March 6, 2021 15:49
@mergify mergify bot dismissed yurishkuro’s stale review March 6, 2021 04:49

Pull request has been modified.

@albertteoh
Copy link
Contributor Author

Might warrant a "breaking change" entry in the changelog

👍 Breaking change entry added.

@yurishkuro
Copy link
Member

the ES integration tests don't seem to like this change

albertteoh and others added 5 commits March 7, 2021 22:52
},
others: make(map[string]*namespaceConfig, len(otherNamespaces)),
}

// Other namespaces need to be explicitly enabled.
defaultConfig.Enabled = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should fix both the failing ES integration tests and Crossdock builds.

The cause was due to archive mode being enabled by default and attempting to either ping a non-existent ES for a version (failing ES integration test) or attempting to communicate with ES on the wrong (default) IP address (failing Crossdock builds).

The original behaviour was to have archive mode disabled.

@yurishkuro yurishkuro merged commit 6072ef2 into jaegertracing:master Mar 10, 2021
@jpkrohling jpkrohling added this to the Release 1.23.0 milestone Jun 4, 2021
@albertteoh albertteoh deleted the fix-archive-flag-init-defaults-0 branch June 21, 2021 05:53
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.

Fix archive default values reported for ES and Cassandra
3 participants