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 elasticsearch storage date format config. #1325

Merged
merged 3 commits into from
Jun 24, 2022

Conversation

sniperking1234
Copy link
Contributor

@sniperking1234 sniperking1234 commented Dec 1, 2020

Signed-off-by: Chen Zhengwei chenzhengwei@inspur.com

Which problem is this PR solving?

Short description of the changes

  • User can configure the date separator to change the date format.
  • Default date formate is yyyy-MM-dd, it won't affect the old version.

Depends on: jaegertracing/jaeger#2637 jaegertracing/spark-dependencies#101

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #1325 (ca3eec0) into main (40194c2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ca3eec0 differs from pull request most recent head 3a62f7a. Consider uploading reports for the commit 3a62f7a to get more accurate results

@@           Coverage Diff           @@
##             main    #1325   +/-   ##
=======================================
  Coverage   88.23%   88.24%           
=======================================
  Files         100      100           
  Lines        6385     6387    +2     
=======================================
+ Hits         5634     5636    +2     
  Misses        553      553           
  Partials      198      198           
Impacted Files Coverage Δ
pkg/cronjob/es_rollover.go 79.51% <100.00%> (+0.12%) ⬆️
pkg/cronjob/spark_dependencies.go 83.46% <100.00%> (+0.13%) ⬆️

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 40194c2...3a62f7a. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Sounds reasonable. @kevinearls, given that you are working closer with ES nowadays, could you please review this?

@jpkrohling
Copy link
Contributor

@sniperking1234, is this feature available at Jaeger already?

@kevinearls
Copy link
Contributor

@jpkrohling this depends on jaegertracing/jaeger#2637 and jaegertracing/spark-dependencies#101

@sniperking1234 After those two are merged I'd like to see an E2E test for this. You could either add a case to test/e2e/elasticsearch_test.go or modify one of the two test cases that use indices.

@jpkrohling
Copy link
Contributor

What's the status of this PR?

@sniperking1234
Copy link
Contributor Author

@jpkrohling The dependent commit is still awaiting review. jaegertracing/spark-dependencies#101

Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

sniperking1234 and others added 2 commits June 24, 2022 10:53
Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com>

Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus frzifus enabled auto-merge (squash) June 24, 2022 12:18
Copy link
Contributor

@kevinearls kevinearls left a comment

Choose a reason for hiding this comment

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

LGTM

@frzifus frzifus merged commit 3a3bdc6 into jaegertracing:main Jun 24, 2022
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.

elasticsearch storage can not customize the date format
4 participants