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.

Sign up for GitHub

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

Jump to bottom

feat: don't throw error if unable to enrich metadata #2608

Merged
merged 8 commits into from May 17, 2023
Merged

feat: don't throw error if unable to enrich metadata #2608

merged 8 commits into from May 17, 2023

Conversation

nityanandagohain
Copy link
Member

Fixes #2601

@github-actions github-actions bot added the enhancement New feature or request label Apr 21, 2023
@nityanandagohain
Copy link
Member Author

Please hold the review for this PR, as there are some changes that needs to be added by @makeavish and then I will update this PR.

@CLAassistant
Copy link

CLAassistant commented Apr 21, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@nityanandagohain
Copy link
Member Author

This PR is ready for review

makeavish
makeavish previously approved these changes Apr 25, 2023
View reviewed changes
@nityanandagohain
Copy link
Member Author

@srikanthccv , please have a look at this PR, we can merge it if there are no concerns.

@nityanandagohain nityanandagohain changed the title feat: don't throw error is unable to enrich metadata feat: don't throw error if unable to enrich metadata May 2, 2023
srikanthccv
srikanthccv reviewed May 2, 2023
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

I see enrich being used in several places, can we refactor it to add the missing type, datatype at the intial step buildLogsQuery or somewhere and all the sub functions can safely assume they are set.

pkg/query-service/app/logs/v3/query_builder.go Outdated Show resolved Hide resolved
@srikanthccv
Copy link
Member

Bumping, let me know what you think about the comment.

@nityanandagohain
Copy link
Member Author

nityanandagohain commented May 12, 2023 •
edited

Hey @srikanthccv , sorry I missed this. Yes, it makes sense.

Even @makeavish suggested something like this that event before the fields reach the parser we can enrich. Let me create a separate issue for that.

#2687

srikanthccv
srikanthccv approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

It was confusing to follow where enrichment is needed vs where it is done. Otherwise LGTM. Please make sure to test it.

pkg/query-service/app/logs/v3/query_builder.go Show resolved Hide resolved
@nityanandagohain nityanandagohain merged commit d1a256a into develop May 17, 2023
9 of 11 checks passed
@nityanandagohain nityanandagohain deleted the issue_2601 branch May 17, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv approved these changes

@makeavish makeavish makeavish left review comments

Assignees
No one assigned
Labels
codeball:needs-careful-review Codeball enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Logs] Don't send error if metadata of a key is not found
4 participants
@nityanandagohain @CLAassistant @srikanthccv @makeavish