Add es-index-cleaner golang implementation #3192

pavolloffay · 2021-08-06T16:11:44Z

Updates #3179

Sharing this early to get some reviews. The only missing part in this PR is to increase the test coverage.

Future PRs:

add support for user/pass and TLS
add docker build and include this in ES integration tests
remove old python implementation

codecov · 2021-08-09T12:13:08Z

Codecov Report

Merging #3192 (7907934) into master (4019fe9) will increase coverage by 0.01%.
The diff coverage is 95.20%.

@@            Coverage Diff             @@
##           master    #3192      +/-   ##
==========================================
+ Coverage   95.93%   95.94%   +0.01%     
==========================================
  Files         239      242       +3     
  Lines       14661    14786     +125     
==========================================
+ Hits        14065    14187     +122     
- Misses        519      521       +2     
- Partials       77       78       +1

Impacted Files	Coverage Δ
cmd/es-index-cleaner/app/index_client.go	`91.78% <91.78%> (ø)`
cmd/es-index-cleaner/app/flags.go	`100.00% <100.00%> (ø)`
cmd/es-index-cleaner/app/index_filter.go	`100.00% <100.00%> (ø)`
cmd/query/app/static_handler.go	`95.80% <0.00%> (-1.80%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`96.21% <0.00%> (+0.70%)`	⬆️
cmd/query/app/server.go	`95.58% <0.00%> (+1.47%)`	⬆️

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 4019fe9...7907934. Read the comment docs.

pavolloffay · 2021-08-10T09:44:24Z

Added TLS and basic auth support to the PR.

pavolloffay · 2021-08-10T11:31:49Z

The coverage is weird, it should be green:

 go test -coverprofile=cover.out  ./cmd/es-index-cleaner...                                            ploffay@fedora
?   	github.com/jaegertracing/jaeger/cmd/es-index-cleaner	[no test files]
ok  	github.com/jaegertracing/jaeger/cmd/es-index-cleaner/app	0.004s	coverage: 96.6% of statements

It also says that there is a regression in static handler which I didn't touch.

Signed-off-by: Pavol Loffay <[email protected]>

cmd/es-index-cleaner/app/index_client.go

rubenvp8510 · 2021-08-11T01:00:35Z

cmd/es-index-cleaner/app/index_client.go

+//         indices: jaeger-span-archive-000001
+func (i *IndicesClient) GetJaegerIndices(prefix string) ([]Index, error) {
+	prefix += "jaeger-*"
+	r, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/%s?flat_settings=true&filter_path=*.aliases,*.settings", i.Endpoint, prefix), nil)


Question, do we need to URL encode the prefix?

doesn't the NewRequest encode the url?

not sure, I don't think it is important. I was just wondering what happen if the prefix is for instance pattern is jaeger/tracing-*

cmd/es-index-cleaner/app/index_client.go

jpkrohling · 2021-08-10T12:24:17Z

cmd/es-index-cleaner/app/index_client.go

+		return nil, fmt.Errorf("failed to query Jaeger indices: %q", err)
+	}
+
+	if response.StatusCode != 200 {


You might want to accept the whole 200 class (like 204), instead of specifically the 200.

Also, you might want to have a list of errors that are retriable, making this more resilient.

I want to be strict here ES returns only 200.

Retries can be added later as a separate feature.

cmd/es-index-cleaner/app/index_client.go

cmd/es-index-cleaner/app/index_filter_test.go

jpkrohling · 2021-08-11T08:53:01Z

cmd/es-index-cleaner/main.go

+)
+
+func main() {
+	logger, _ := zap.NewDevelopment()


Is this appropriate?

copy paste from esmapping-generator main

cmd/es-index-cleaner/main.go

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2021-08-11T09:49:52Z

PR updated

rubenvp8510

LGTM

jpkrohling

I'm OK to give it a try!

albertteoh · 2021-08-12T10:03:13Z

cmd/es-index-cleaner/app/flags.go

+	flags.String(indexPrefix, "", "Index prefix")
+	flags.Bool(archive, false, "Whether to remove archive indices")
+	flags.Bool(rollover, false, "Whether to remove indices created by rollover")
+	flags.Int(timeout, 120, "Number of seconds to wait for master node response")


Is the reason for using seconds instead of Duration for compatibility with the python version?

albertteoh · 2021-08-12T10:08:35Z

cmd/es-index-cleaner/app/index_client.go

+	}
+	var indicesInfo map[string]indexInfo
+	if err = json.Unmarshal(body, &indicesInfo); err != nil {
+		return nil, fmt.Errorf("failed to query indices and unmarshall response body: %q: %w", body, err)


Suggested change

return nil, fmt.Errorf("failed to query indices and unmarshall response body: %q: %w", body, err)

return nil, fmt.Errorf("failed to query indices and unmarshal response body: %q: %w", body, err)

albertteoh · 2021-08-12T10:10:52Z

cmd/es-index-cleaner/app/index_client_test.go

+			errContains:  "failed to query indices: request failed, status code: 400",
+		},
+		{
+			name:         "unmarshall error",


Suggested change

name: "unmarshall error",

name: "unmarshal error",

albertteoh · 2021-08-12T10:11:01Z

cmd/es-index-cleaner/app/index_client_test.go

+			name:         "unmarshall error",
+			responseCode: http.StatusOK,
+			response:     "AAA",
+			errContains:  `failed to query indices and unmarshall response body: "AAA"`,


Suggested change

errContains: `failed to query indices and unmarshall response body: "AAA"`,

errContains: `failed to query indices and unmarshal response body: "AAA"`,

pavolloffay requested a review from a team as a code owner August 6, 2021 16:11

pavolloffay requested a review from joe-elliott August 6, 2021 16:11

pavolloffay force-pushed the es-cleaner-golang branch 2 times, most recently from e4c0d79 to 0cd66b9 Compare August 10, 2021 09:44

Add golang implementation for es-index-cleaner

18aea51

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay force-pushed the es-cleaner-golang branch from 03e9826 to 18aea51 Compare August 10, 2021 11:34

rubenvp8510 reviewed Aug 11, 2021

View reviewed changes

jpkrohling reviewed Aug 11, 2021

View reviewed changes

review comments

7907934

Signed-off-by: Pavol Loffay <[email protected]>

rubenvp8510 approved these changes Aug 12, 2021

View reviewed changes

jpkrohling approved these changes Aug 12, 2021

View reviewed changes

pavolloffay merged commit 94bc987 into jaegertracing:master Aug 12, 2021

albertteoh approved these changes Aug 12, 2021

View reviewed changes

jpkrohling added this to the v1.26.0 milestone Aug 23, 2021

maksim-paskal mentioned this pull request Aug 24, 2021

es-index-cleaner:latest is not working as expected #3228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add es-index-cleaner golang implementation #3192

Add es-index-cleaner golang implementation #3192

pavolloffay commented Aug 6, 2021

codecov bot commented Aug 9, 2021 •

edited

Loading

pavolloffay commented Aug 10, 2021

pavolloffay commented Aug 10, 2021

rubenvp8510 Aug 11, 2021

pavolloffay Aug 11, 2021

rubenvp8510 Aug 12, 2021 •

edited

Loading

jpkrohling Aug 10, 2021

jpkrohling Aug 10, 2021

pavolloffay Aug 11, 2021

jpkrohling Aug 11, 2021

pavolloffay Aug 11, 2021

pavolloffay commented Aug 11, 2021

rubenvp8510 left a comment

jpkrohling left a comment

albertteoh Aug 12, 2021

pavolloffay Aug 12, 2021

albertteoh Aug 12, 2021

albertteoh Aug 12, 2021

albertteoh Aug 12, 2021

	return nil, fmt.Errorf("failed to query indices and unmarshall response body: %q: %w", body, err)
	return nil, fmt.Errorf("failed to query indices and unmarshal response body: %q: %w", body, err)

	errContains: `failed to query indices and unmarshall response body: "AAA"`,
	errContains: `failed to query indices and unmarshal response body: "AAA"`,

Add es-index-cleaner golang implementation #3192

Add es-index-cleaner golang implementation #3192

Conversation

pavolloffay commented Aug 6, 2021

codecov bot commented Aug 9, 2021 • edited Loading

Codecov Report

pavolloffay commented Aug 10, 2021

pavolloffay commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Aug 11, 2021

rubenvp8510 left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2021 •

edited

Loading

rubenvp8510 Aug 12, 2021 •

edited

Loading