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

NETOBSERV-1017 extract timebased indexKeys #450

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

jpinsonneau
Copy link
Collaborator

@jpinsonneau jpinsonneau commented Jul 6, 2023

In order to get top Pod / Service talkers this PR implements IndexKeys as an array for timebased stage.

It also add the capability to index on non string fields using ConvertToString utils

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #450 (25842d7) into main (7dde343) will increase coverage by 3.27%.
The diff coverage is 70.88%.

@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
+ Coverage   62.85%   66.12%   +3.27%     
==========================================
  Files          96       94       -2     
  Lines        6916     6867      -49     
==========================================
+ Hits         4347     4541     +194     
+ Misses       2325     2066     -259     
- Partials      244      260      +16     
Flag Coverage Δ
unittests 66.12% <70.88%> (+3.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/api/extract_timebased.go 100.00% <ø> (ø)
pkg/utils/convert.go 73.70% <27.27%> (-5.35%) ⬇️
pkg/pipeline/extract/timebased/timebased.go 82.05% <50.00%> (-9.13%) ⬇️
pkg/pipeline/extract/timebased/tables.go 83.01% <86.95%> (-0.32%) ⬇️
pkg/pipeline/extract/timebased/filters.go 84.54% <100.00%> (+0.73%) ⬆️
pkg/test/utils.go 74.49% <100.00%> (+1.83%) ⬆️

... and 10 files with indirect coverage changes

@jpinsonneau jpinsonneau marked this pull request as ready for review July 6, 2023 15:56
case string:
return i, nil
default:
v := reflect.ValueOf(unk)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could't we use fmt.Sprintf(%v) to convert anything to a string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good, b8cc564

t[k] = values[i]
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Once you've added in the individual fields, I don't see the need to keep the key, operation_key, and operation_result fields.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes exactly; done in 25842d7

@jpinsonneau
Copy link
Collaborator Author

Thanks @KalmanMeth 🥳

@jpinsonneau
Copy link
Collaborator Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpinsonneau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 08e1c0c into netobserv:main Jul 24, 2023
8 checks passed
@jotak
Copy link
Member

jotak commented Jul 31, 2023

@KalmanMeth @jpinsonneau should we mark that as a breaking change? (the API is preserved however the output fields are different if I understand correctly (operation_key and operation_result being removed)

I'm not concerned for netobserv, but other FLP users

@jpinsonneau jpinsonneau added the breaking-change This pull request has breaking changes. They should be described in PR description. label Aug 1, 2023
@jpinsonneau
Copy link
Collaborator Author

@KalmanMeth @jpinsonneau should we mark that as a breaking change? (the API is preserved however the output fields are different if I understand correctly (operation_key and operation_result being removed)

I'm not concerned for netobserv, but other FLP users

Added the label on that PR. Check #450 (comment) discussion

@KalmanMeth
Copy link
Collaborator

@jpinsonneau For completeness we should mark it as a breaking change (although you did maintain back compatibility for the current implementation, indicating deprecation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved breaking-change This pull request has breaking changes. They should be described in PR description. lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants