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

Date filter, logging and types #26

Merged
merged 6 commits into from
Jul 10, 2024

Conversation

shashank-boyapally
Copy link
Collaborator

Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization
  • Documentation Update

Description

Added logging for common usage, date filter for orion's new feature of lookback and types to methods

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please describe the System Under Test.
  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Copy link
Member

@jtaleric jtaleric left a comment

Choose a reason for hiding this comment

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

Can we add a lookback test?

@paigerube14
Copy link
Collaborator

Can we add a lookback test?

speaking of, I see there are unit tests in a workflow. But I don't see it running on this pr. Is the workflow run part properly configured?

@shashank-boyapally
Copy link
Collaborator Author

Unit tests are performed on push, in the fork branch as of now, we can configure it to run for pull requests too.

@paigerube14
Copy link
Collaborator

@shashank-boyapally do you see the test results on your fork then? I think it would be more helpful to show that result in the PR itself so we can know if everything is passing before merge

Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Copy link
Member

@jtaleric jtaleric left a comment

Choose a reason for hiding this comment

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

nice! lgtm! Thanks!

Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Copy link
Member

@jtaleric jtaleric left a comment

Choose a reason for hiding this comment

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

approving again ;)

Copy link
Collaborator

@paigerube14 paigerube14 left a comment

Choose a reason for hiding this comment

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

lgtm

@jtaleric jtaleric merged commit 69145e4 into cloud-bulldozer:main Jul 10, 2024
5 checks passed
meta["networkType"] = "OVNKubernetes"
meta["benchmark.keyword"] = "cluster-density-v2"
# meta['encrypted'] = "true"
# meta['ipsec'] = "false"
# meta['fips'] = "false"

uuids = match.get_uuid_by_metadata(meta)
print("All uuids",len(uuids))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these just debugs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep this is just a utility for functionality testing.

import sys


class SingletonLogger:
Copy link
Contributor

@vishnuchalla vishnuchalla Jul 10, 2024

Choose a reason for hiding this comment

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

I don't think we need a singleton class here. It just as simple as updating a config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. The thought behind this was as follows: having a logger setup in Matcher class created duplicate logs of the same log-output, especially in daemon mode where multiple Matcher objects would be created as when the request would come in and multiple formatters were being attached to the logger which led to duplicate logs. To mitigate this a temporary solution was to check if the logger existed previously. I transferred this logic to a class so that it can be used in Orion as well. The SingletonLogger here also updates the config using methods and also keeps track of only having a single logger.

Copy link
Contributor

@vishnuchalla vishnuchalla Jul 10, 2024

Choose a reason for hiding this comment

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

logging is singleton by default. All I am saying is, I think we need not write a custom logging class instead of just using the existing ones.

"ignore", category=UserWarning, message=".*Connecting to.*verify_certs=False.*"
)

match = Matcher(index="perf_scale_ci*", verify_certs=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to change it to ospst-* as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently this utility works on the qe instance...

Copy link
Member

Choose a reason for hiding this comment

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

Nope - as that would require having access to internal resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. Understood.

if len(uuids) == 0:
print("No UUID present for given metadata")
sys.exit()
runs = match.match_kube_burner(uuids)
runs = match.match_kube_burner(uuids,"ripsaw-kube-burner*")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as well ospst-*

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.

4 participants