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

Add Git commit and build info metrics #4286

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ardetrick
Copy link

See #3154.

I was a bit unsure how closely I should be following the Spring Boot PR referenced in the issue. This implementation is more explicit in which attributes are allowed and how they are supplied to the binder. But there could be an alternative approach that builds the info objects using an instance of Properties.

Other naming specific questions I have are:

  • Is unknown an acceptable default value for the tag? Would you rather it go in a specific class rather than in MeterBinder?
  • Is GitInfo preferable to GitCommitInfo?
  • Are the tag names inline with expectations?

@pivotal-cla
Copy link

@ardetrick Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@ardetrick Thank you for signing the Contributor License Agreement!


private final BuildInfo buildInfo;

public BuildInfoMetrics(BuildInfo buildInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

May be one more constructor to add user provided tags to metrics?

Copy link
Author

Choose a reason for hiding this comment

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

This has been added.

.tag("artifact", buildInfo.getArtifact().orElse(DEFAULT_TAG_VALUE))
.tag("name", buildInfo.getName().orElse(DEFAULT_TAG_VALUE))
.tag("version", buildInfo.getVersion().orElse(DEFAULT_TAG_VALUE))
.tag("timestamp", buildInfo.getTimestamp().map(Instant::toString).orElse(DEFAULT_TAG_VALUE))
Copy link
Contributor

Choose a reason for hiding this comment

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

My initial thoughts on the timestamp was whether it is a good candidate for metrics. But, I guess it should be fine since it will most probably be tied to versions and timestamp is more of an informative tag than anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

But is this really for tracking build info of dependencies? or the service itself?

Copy link
Author

Choose a reason for hiding this comment

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

I think it could be used for either the service and/or one or many dependencies. This implementation does not impose an opinion. Ultimately it was added to try and meet parity with the SpringBoot equivalent class.

One use case for a dependency would be to use this metric to assert that "all production services are using SpringBoot version 3.1.x or a version with a timestamp value from the last 3 months."

One could case for the service might be to use this metric in combination with something like "deployment.info" (which does not exist in Micrometer right now, but for the purposes of this discussion, lets say it captures at least a timestamp value of when the runtime was deployed). You could then use these two metrics together to understand how long it takes to get an application into production starting from the time the deployable artifact was created. This example is a bit more complicated and has it's flaws (such as rollbacks, rolling deployments, auto-scaling) but shared it as a potential idea.

Ultimately I agree with you, the timestamp value may end up being more informational than anything but I could imagine use cases for it to be useful and it meets SpringBoot feature parity..

@ardetrick
Copy link
Author

@lenin-jaganathan thank you for your time and effort in reviewing this PR and the related issue. Please let me know if there are additional questions, discussions, or changes.

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