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

[approvaltest] Ignore dynamic fields from sort fields #61

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

lahsivjar
Copy link
Contributor

@lahsivjar lahsivjar commented Jun 5, 2024

The approval tests doesn't handle dynamic fields correctly, the fields are sorted before dynamic is handled. Due to this, 2 issues could happen:

  1. If a field is passed as dynamic but it is also in the sort fields then the resulting sort would be indeterministic.
  2. If all the sort fields are identical then the fields are compared using bytes.Compare this will again result in indeterministic sorts if the dynamic fields are randomly generated.

Both these issues are fixed in this PR by rewriting the dynamic fields before sorting and comparing for approvals.

Required to fix the failures in elastic/apm-server#13196

pkg/approvaltest/sort.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@lahsivjar lahsivjar requested a review from kruskall June 5, 2024 15:42
@lahsivjar lahsivjar enabled auto-merge (squash) June 5, 2024 16:03
@lahsivjar lahsivjar requested a review from a team June 5, 2024 16:03
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Could this PR be simplified by pulling the rewrite loops out of approveEventDocs and approveFields? i.e. change ApproveEvents and ApproveFields to first rewrite, then sort, and finally approve.

@lahsivjar
Copy link
Contributor Author

Could this PR be simplified by pulling the rewrite loops out of approveEventDocs and approveFields?

@axw This was my first thought and I spent some time into trying to refactor code as per this but the code I came up with was not optimum and required copying the raw fields and the raw source both in-memory. My previous effort was based on not decoding the data first but I think we might be able to do this if we decode to JSON first and then parse dynamic fields. I will give this a shot.

@lahsivjar
Copy link
Contributor Author

@axw Fixed the logic to rewrite before sort. PTAL.

@lahsivjar lahsivjar requested a review from axw June 6, 2024 14:38
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks good, just one query

for _, field := range dynamic {
existing := gjson.GetBytes(doc, field)
field = strings.ReplaceAll(field, ".", "\\.")
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to work for _source as well as fields? We might need to switch based on whether it's ApproveEvents vs. ApproveFields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@axw You are right, it wouldn't have worked with ApproveEvents, I have fixed it now. Thanks for catching this.

@lahsivjar lahsivjar requested a review from axw June 7, 2024 10:25
@lahsivjar lahsivjar merged commit a4f490d into elastic:main Jun 7, 2024
3 checks passed
@lahsivjar lahsivjar deleted the jaeger-error-id branch June 7, 2024 12:24
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.

3 participants