Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Init version #1

Merged
merged 11 commits into from
Nov 26, 2020
Merged

Init version #1

merged 11 commits into from
Nov 26, 2020

Conversation

MalloZup
Copy link
Contributor

@MalloZup MalloZup commented Nov 18, 2020

Description:

This PR port the exporter from my home repo to suse namespace.

It is a initial functional version of the exporter.

Many things were left over because for an initial version is ok to be concentrated to the essential part of it.

Also due to the rationale of this exporter, and prios we have I didn't invested much time on the Filesystem layout of the project , golang packages , etc this we can do later on.

I'm Adding Angela as CC reviewer just in case she spot something not saptune conform :)

@stefanotorresi
Copy link
Contributor

stefanotorresi commented Nov 18, 2020

hey @MalloZup great stuff, but could you please add at least one functional test like the ones we have in the other exporters? You can take the sap_host_exporter as the most up to date example. Otherwise I can't really see how the metrics are structured, without actually running the thing.

Copy link
Contributor

@stefanotorresi stefanotorresi left a comment

Choose a reason for hiding this comment

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

Hey here are a few other of things I'd like to see:

  • We need to provide metrics docs, otherwise the semantics of the exporter are impossible to understand. Even just some examples with a few notes could suffice, you don't necessarily need to make them super-comprehensive from the start. We'll uniform the docs with the other exporters later, but we need at least some examples to start with and reason about.
  • I think we should rename all the metric labels according to the naming convention we follow in all the other exporters, which is snake_case rather than PascalCase.
  • Given our past experience, we shall try harder to avoid disappearing metrics, and rather list disabled things with an explicit 0, if possible.

@MalloZup
Copy link
Contributor Author

MalloZup commented Nov 19, 2020

Hi @stefanotorresi thx for review. I will let this PR currently open a bit because i'm focusing on other things, currently.

I agree on the comments , I didn't had time to implement the missing things yet

Will address comments later on thx

@stefanotorresi
Copy link
Contributor

no worries, there is no rush.

@MalloZup
Copy link
Contributor Author

follow-up: #4

@MalloZup
Copy link
Contributor Author

@stefanotorresi I adressed most of your comment with a follow-up issue for the metrics "disappering". For that particular case I think we could investigate if we can set them to 0, something I will look at.

I think for a first releases 0.1 this PR is ok now. (test + doc ) added.

Copy link

@angelabriel angelabriel left a comment

Choose a reason for hiding this comment

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

Great work, looks really good for me.
Many thanks for the work you spend to make saptune visible.

For the future we can add more saptune operations to the exporter and the new API Sören and I working on will help to make it a bit easier to adapt new stuff.
But be in mind, saptune is not providing dynamic data, the values are more of static nature. So it's not a typical metric.

@MalloZup
Copy link
Contributor Author

thx @angelabriel

Copy link
Contributor

@stefanotorresi stefanotorresi left a comment

Choose a reason for hiding this comment

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

Perfect, @MalloZup! Top work!

@stefanotorresi stefanotorresi merged commit 5482d0c into master Nov 26, 2020
@stefanotorresi stefanotorresi deleted the init-version branch November 26, 2020 12:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants