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

Add custom metrics config option #20

Merged
merged 24 commits into from
Sep 19, 2024
Merged

Add custom metrics config option #20

merged 24 commits into from
Sep 19, 2024

Conversation

Deezzir
Copy link
Contributor

@Deezzir Deezzir commented Sep 16, 2024 •
edited
Loading

  • Adds a new snap config option - dcgm-exporter-metrics-file.
  • Adds a test for the new configuration.
  • Divides tests into two classes: TestDCGMComponents and TestDCGMConfigs.

The user can set that config with the filename of the metrics file placed in /var/snap/dcgm/common prior. The dcgm-exporter wrapper will then check if the file exists in the directory above and supply it to the service.

The custom metrics file we developed will not be added alongside with the PR. It will be moved to the charm later when the feature is merged.

@Deezzir Deezzir marked this pull request as ready for review September 17, 2024 20:12
@Deezzir Deezzir requested a review from a team as a code owner September 17, 2024 20:12
gabrielcocenza
gabrielcocenza requested changes Sep 17, 2024
View reviewed changes
Copy link
Member

@gabrielcocenza gabrielcocenza left a comment

Choose a reason for hiding this comment

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

It's looking good.

It's a personal preference but one of the highlights to me from pytest is that it's not necessary to create classes like when using unittest. I prefer not having classes, but no strong opinion

snap/hooks/configure Outdated Show resolved Hide resolved
snap/hooks/configure Outdated Show resolved Hide resolved
snap/local/files/run_dcgm_exporter.sh Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
aieri
aieri requested changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@aieri aieri 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, changes requested simply because I agree with Gabriel's comments

snap/local/files/run_dcgm_exporter.sh Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
@Deezzir Deezzir mentioned this pull request Sep 18, 2024
Merged
jneo8
jneo8 requested changes Sep 18, 2024
View reviewed changes
Copy link

@jneo8 jneo8 left a comment

Choose a reason for hiding this comment

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

One small suggestion

tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
aieri
aieri requested changes Sep 18, 2024
View reviewed changes
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
@Deezzir Deezzir requested a review from aieri September 18, 2024 22:53
aieri
aieri previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@aieri aieri left a comment

Choose a reason for hiding this comment

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

nice!

samuelallan72
samuelallan72 previously approved these changes Sep 19, 2024
View reviewed changes
aieri
aieri previously approved these changes Sep 19, 2024
View reviewed changes
jneo8
jneo8 previously approved these changes Sep 19, 2024
View reviewed changes
gabrielcocenza
gabrielcocenza requested changes Sep 19, 2024
View reviewed changes
Copy link
Member

@gabrielcocenza gabrielcocenza left a comment

Choose a reason for hiding this comment

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

Nice job!
Just some small details to polish

tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
tests/functional/test_snap_dcgm.py Outdated Show resolved Hide resolved
@Deezzir Deezzir dismissed stale reviews from jneo8, aieri, and samuelallan72 via 4c09171 September 19, 2024 14:28
@Deezzir Deezzir merged commit c3b3a91 into canonical:main Sep 19, 2024
5 checks passed
@Deezzir Deezzir deleted the metrics-config branch September 19, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabrielcocenza gabrielcocenza gabrielcocenza approved these changes

@aieri aieri aieri approved these changes

@Pjack Pjack Awaiting requested review from Pjack Pjack is a code owner automatically assigned from canonical/soleng-reviewers

@rgildein rgildein Awaiting requested review from rgildein rgildein is a code owner automatically assigned from canonical/soleng-reviewers

@chanchiwai-ray chanchiwai-ray Awaiting requested review from chanchiwai-ray chanchiwai-ray is a code owner automatically assigned from canonical/soleng-reviewers

@samuelallan72 samuelallan72 Awaiting requested review from samuelallan72

@jneo8 jneo8 Awaiting requested review from jneo8

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Deezzir @aieri @samuelallan72 @jneo8 @gabrielcocenza