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

Prometheus - #589

Merged
gdbelvin merged 3 commits into
google:masterfrom
gdbelvin:prometheus
Jun 19, 2017
Merged

gdbelvin merged 3 commits into
google:masterfrom
gdbelvin:prometheus

Conversation

@gdbelvin

@gdbelvin gdbelvin commented May 23, 2017 •

Copy link
Copy Markdown
Contributor

Merge #578 first.

Adds prometheus monitoring

  • grpc statistics
  • signer statistics
  • prometheus configuration

Fixes #610

@gdbelvin
gdbelvin requested a review from cesarghali May 23, 2017 23:17
@gdbelvin
gdbelvin force-pushed the prometheus branch 2 times, most recently from 1568980 to 40b74ac Compare June 13, 2017 14:33
@gdbelvin
gdbelvin requested a review from liamsi June 13, 2017 14:34
@google google deleted a comment from codecov-io Jun 13, 2017
@codecov-io

codecov-io commented Jun 13, 2017 •

Copy link
Copy Markdown

Codecov Report

Merging #589 into master will decrease coverage by 0.18%.
The diff coverage is 23.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
- Coverage   56.95%   56.76%   -0.19%     
==========================================
  Files          35       35              
  Lines        2472     2489      +17     
==========================================
+ Hits         1408     1413       +5     
- Misses        775      787      +12     
  Partials      289      289
Impacted Files Coverage Δ
core/signer/signer.go 12% <23.8%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe386b3...38b07ed. Read the comment docs.


return resp, err
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These changes will also resolve #610 👍

@liamsi liamsi left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code LGTM besides a few nits. I'll manually test it locally soon.

Can you please do a git push -f from your local branch before addressing any comments? This will remove the last commit and googlebot's complain about the CLA.

Comment thread core/signer/signer.go Outdated

// Put SignedMapHead in an append only log.
if err := s.sths.Write(ctx, s.logID, setResp.MapRoot.MapRevision, setResp.MapRoot); err != nil {
// TODO: sths.Write currently hangs.

@liamsi liamsi Jun 15, 2017 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

// TODO: sths.Write currently hangs.
Is this still true? If yes, change to TODO(name): else remove.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread docker-compose.yml Outdated
image: prom/prometheus
volumes:
- ./prometheus.yml:/etc/prometheus/prometheus.yml
- ./prometheus.rules:/etc/prometheus/prometheus.rules

@liamsi liamsi Jun 15, 2017 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we just ADD/COPY these files in a separate Dockerfile? The prometheus container would be self-contained and this would save us some pain when running this on GCE with kubernetes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread core/signer/signer.go Outdated
)

var (
timeout = 10 * time.Second

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this timeout be a commandline flag? Are 10s always a reasonable timeout?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed and replaced with minInterval

@gdbelvin
gdbelvin force-pushed the prometheus branch 2 times, most recently from 40b74ac to 027c19a Compare June 15, 2017 16:09
Comment thread docker-compose.yml Outdated
VRF_PUB: /kt/genfiles/vrf-pubkey.pem
SIGN_KEY: /trillian/log-rpc-server.privkey.pem
SIGN_KEY_PW: towel
SIGN_PERIOD: 5s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The corresponding flag got renamed to --min-period for the signer. I would suggest to name this MIN_SIGN_PERIOD (Already fixed in #607: https://github.com/Liamsi/keytransparency/blob/6d3dde3ce28615df44bb80ac2076307c6e572362/cmd/keytransparency-signer/Dockerfile#L26).
If this gets merged before #607, please update the flag in the Dockerfile (otherwise docker-compose won't succeed starting the kt-signer).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@google google deleted a comment from googlebot Jun 19, 2017
@google google deleted a comment from googlebot Jun 19, 2017
gdbelvin added 3 commits June 19, 2017 12:47
- grpc interceptor for kt-server.
- metrics framework for kt-signer.
- renamed main files to match go standards.

Closes google#481
- Adds signer metrics
- Closes google#587
@gdbelvin

Copy link
Copy Markdown
Contributor Author

Updated PTAL

@liamsi liamsi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! LGTM

@gdbelvin
gdbelvin merged commit 2128c1d into google:master Jun 19, 2017
@gdbelvin
gdbelvin deleted the prometheus branch June 19, 2017 14:54
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