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

Extract prometheus metrics #1 #256

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Extract prometheus metrics #1 #256

merged 1 commit into from
Jan 15, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Jan 14, 2021

Related to #255
This is the first part of larger refactoring.

  • move Dig to utils
  • move prettyPrint() to utils and rename to ToJSON()
  • cover ToJSON() by tests
  • move environment variable POD_NAMESPACE to depresolver
  • extend depresolver tests
  • extend depresolver validators for namespace validator
  • extract Prometheus metrics into standalone module
  • extend Prometheus metrics with Register(),Unregister()
  • remove controllers/metrics.go
  • fix controller tests
  • remove dead registry/k3s.yaml file (pushed by mistake)

TODO:

  • dnsupdate refactor, to provider pattern, will come in the next PR
  • finally, some utils will go into gopkg
  • IMetrics introduction (should be implemented with Expose advanced metrics #124)

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Look good, but not sure if ToJSON is properly reflecting semantics... Originally the function was created just for readable struct printing...

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

I'd probably separate Dig and prettyPrint refactoring to different PRs, as they are out of context of this PR

@kuritka
Copy link
Collaborator Author

kuritka commented Jan 15, 2021

Look good, but not sure if ToJSON is properly reflecting semantics... Originally the function was created just for readable struct printing...

I see, but function accepts interface so can be used to any type. Of course I can limit to structures only, and return error if passed argument is not structure.
Do you think rename may help ? What about ToString(a interface{}) string ?

str{"Foo", 007} => 

{
	"Name": "Foo",
	"Value": 7
}

123 => "123"
true => "true"
nil => "null"  // marshalling implicitly returns this, but can be changed. 

@kuritka kuritka force-pushed the refactor-metrics branch 2 times, most recently from 7a83bea to d4c9f8b Compare January 15, 2021 09:43
@kuritka
Copy link
Collaborator Author

kuritka commented Jan 15, 2021

I'd probably separate Dig and prettyPrint refactoring to different PRs, as they are out of context of this PR

@somaritane, amended.

This is first of PR

 -  move environment variable `POD_NAMESPACE` to `depresolver`
 -  extend depresolver tests
 -  extend depresolver validators for namespace validator
 -  Extract Prometheus metrics into standalone module
 -  Extend prometheus metrics
 -  Fix controller tests
 -  remove dead registry/k3s.yaml file (pushed by mistake)

TODO:
  - dnsupdate refactor to provider pattern in next PR
  - fianally, some of utils will go into [gopkg](https://github.com/AbsaOSS/gopkg)
  - IMetrics introduction (#124)
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm 👍

Copy link
Member

@ytsarev ytsarev 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

@kuritka kuritka merged commit 3dbb963 into master Jan 15, 2021
@kuritka kuritka deleted the refactor-metrics branch January 20, 2021 12:00
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.

None yet

3 participants