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

Adds prometheus metrics #63

Merged
merged 6 commits into from
Mar 14, 2022
Merged

Adds prometheus metrics #63

merged 6 commits into from
Mar 14, 2022

Conversation

nkcr
Copy link
Contributor

@nkcr nkcr commented Mar 9, 2022

Depends on dedis/dela#209

For now we mainly have Dela metrics. Additional metrics specific to the d-Voting system will be added later.

nkcr added 2 commits March 9, 2022 11:06
- Changes the script to use a "local" version of memcoin.
@nkcr nkcr requested review from pierluca and jbsv March 9, 2022 10:16
@nkcr nkcr self-assigned this Mar 9, 2022
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

A few minor comments, I only feel strongly about the makefile 😅 looks pretty neat overalls

README.md Outdated
Comment on lines 177 to 178
Build info can be added to the binary with the `ldflags`, at build time.. Infos
are stored on variables in the root `mod.go`. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Build info can be added to the binary with the `ldflags`, at build time.. Infos
are stored on variables in the root `mod.go`. For example:
Build info can be added to the binary with the `ldflags`, at build time.
Infos are stored on variables in the root `mod.go`. For example:

start_test.sh Outdated
Comment on lines 10 to 13
versionFlag="github.com/dedis/d-voting.Version=`git describe --tags`"
timeFlag="github.com/dedis/d-voting.BuildTime=`date +'%d/%m/%y_%H:%M'`"

go build -ldflags="-X $versionFlag -X $timeFlag" ./cli/memcoin
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be in a location that shouldn't be concerned with building.
I'd make this part of the Makefile.

func (m controller) SetCommands(builder node.Builder) {

cmd := builder.SetCommand("metrics")
cmd.SetDescription("interact with the DKG service")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the description here correct? 🤔

Comment on lines +61 to +76
go func() {
errc <- srv.listen()
}()

for i := 0; i < defaultRetry && srv.getAddr() == nil; i++ {
select {
case err := <-errc:
return xerrors.Errorf("failed to listen: %v", err)
case <-time.After(waitRetry):
}
}

srvAddr := srv.getAddr()
if srvAddr == nil {
return xerrors.Errorf("failed to start metric server")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

According to this, the polling seems entirely avoidable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't solve our problem because we are using an external function to create and start the HTTP server. And we don't want the function to panic with a log.Fatal(server.Serve(ln)) .

@nkcr nkcr requested a review from pierluca March 10, 2022 11:07
- use a make file to build
- fixes wrong command description
@nkcr nkcr force-pushed the adds-prometheus-metrics branch from d3974fc to 97efc99 Compare March 10, 2022 11:08
Comment on lines +180 to +185
```sh
versionFlag="github.com/dedis/d-voting.Version=`git describe --tags`"
timeFlag="github.com/dedis/d-voting.BuildTime=`date +'%d/%m/%y_%H:%M'`"

go build -ldflags="-X $versionFlag -X $timeFlag" ./cli/memcoin
```
Copy link
Contributor

Choose a reason for hiding this comment

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

make build should be enough, now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, that's explained the line bellow 😄 . I found it interesting to explain a bit the magic behind build-time variables.

@nkcr
Copy link
Contributor Author

nkcr commented Mar 11, 2022

Waiting on dedis/dela#209 before merging it.

- Removes deprecated protobuf check
@coveralls
Copy link

coveralls commented Mar 14, 2022

Pull Request Test Coverage Report for Build 1979417812

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 13 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 54.624%

Files with Coverage Reduction New Missed Lines %
services/dkg/pedersen/handler.go 13 84.89%
Totals Coverage Status
Change from base Build 1929485243: -0.2%
Covered Lines: 2989
Relevant Lines: 5472

💛 - Coveralls

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@nkcr nkcr merged commit 44fed4a into main Mar 14, 2022
@nkcr nkcr deleted the adds-prometheus-metrics branch March 14, 2022 08:30
ineiti pushed a commit that referenced this pull request Oct 10, 2024
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.

3 participants