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

Implement tap selector #330

Merged
merged 13 commits into from
Aug 1, 2022
Merged

Implement tap selector #330

merged 13 commits into from
Aug 1, 2022

Conversation

leoparente
Copy link
Collaborator

No description provided.

@leoparente leoparente linked an issue Jul 12, 2022 that may be closed by this pull request
@leoparente leoparente self-assigned this Jul 12, 2022
@leoparente leoparente marked this pull request as draft July 12, 2022 13:49
@leoparente
Copy link
Collaborator Author

Changed to draft: still validating manually it as well

@leoparente leoparente marked this pull request as ready for review July 12, 2022 14:38
@leoparente
Copy link
Collaborator Author

Ready to review

RFCs/2021-04-16-76-collection-policies.md Show resolved Hide resolved
src/Taps.cpp Outdated Show resolved Hide resolved
src/Taps.h Outdated Show resolved Hide resolved
src/Policies.cpp Outdated Show resolved Hide resolved
src/Policies.cpp Outdated Show resolved Hide resolved
src/Policies.cpp Outdated Show resolved Hide resolved
@leoparente leoparente requested a review from weyrick July 15, 2022 17:18
@leoparente
Copy link
Collaborator Author

Tap name concatenated to handler name to ensure uniqueness

{
    "default": {
        "default-default-dns1": {
            "dns": {
                "cardinality": {
                    "qname": 0
                },
                "period": {
                    "length": 1,

And it was added as a label for prometheus return value:

# HELP dns_rates_total Rate of all DNS wire packets (combined ingress and egress) per second
# TYPE dns_rates_total summary
dns_rates_total{module="default-default-dns1",policy="default",quantile="0.5",tap="default"} 0
dns_rates_total{module="default-default-dns1",policy="default",quantile="0.9",tap="default"} 0
dns_rates_total{module="default-default-dns1",policy="default",quantile="0.95",tap="default"} 1
dns_rates_total{module="default-default-dns1",policy="default",quantile="0.99",tap="default"} 1

@leoparente
Copy link
Collaborator Author

Added error 422 when there is no match for tap selector:
image

Copy link
Member

@weyrick weyrick left a comment

Choose a reason for hiding this comment

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

so i think it's almost there - but it's also an extremely hard PR to review since lots of code is moved around and it's tricky logic. i think this means we really need good unit tests because this is a critical section of code. i'd also love to find a way to simplify this policy loading code. ideally that means breaking it up into separate methods performing sections of the transaction which are no more than 20-30 lines long, say. but i don't know how realistic that is.

src/Policies.h Outdated Show resolved Hide resolved
src/Policies.h Outdated Show resolved Hide resolved
src/Policies.h Outdated Show resolved Hide resolved
src/Policies.h Outdated Show resolved Hide resolved
@leoparente
Copy link
Collaborator Author

so i think it's almost there - but it's also an extremely hard PR to review since lots of code is moved around and it's tricky logic. i think this means we really need good unit tests because this is a critical section of code. i'd also love to find a way to simplify this policy loading code. ideally that means breaking it up into separate methods performing sections of the transaction which are no more than 20-30 lines long, say. but i don't know how realistic that is.

Agreed. That's why I added new unit tests to validate it, did this based on Code Coverage to ensure to test new code.

I think this PR is a improvement on that policy load which add new two methods to break it a bit. I also agree that we can improve more that... But I think we could do that in another PR

@leoparente leoparente requested a review from weyrick July 27, 2022 21:16
@leoparente leoparente merged commit f2e5d8f into develop Aug 1, 2022
@leoparente leoparente deleted the feature/tap-selector branch August 1, 2022 14:32
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.

RFC: Tap selectors
2 participants