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

Use consistent package space for newrelic registry #104

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Use consistent package space for newrelic registry #104

merged 1 commit into from
Sep 24, 2020

Conversation

ebullient
Copy link
Contributor

I recommend keeping all resources for this registry inside a newrelic package space (rather than overlapping with io.micrometer), as that can be a strong indicator of who wrote (or at least, who is maintaining) the code.

I would start here as a first pass towards an officially maintained 3rd party driver referenced in micrometer docs (I work for none of the parties concerned.. it is only a recommendation based on what I would consider. ;)

@CLAassistant
Copy link

CLAassistant commented Sep 23, 2020

CLA assistant check
All committers have signed the CLA.

@breedx-nr
Copy link
Contributor

Hi @ebullient. Thanks for submitting this. I was around during the inception of this project, and I believe that the original intent was to try and PR this work back into the main micrometer repo...and I think that's the reason those packages were chosen originally.

Clearly it hasn't really gone that route, and so this change probably makes sense. 👍

I do want to chat with some folks here to get consensus around our approach, but this change does seem like the right thing.

Thanks again, we will let you know.

@jasonjkeller
Copy link
Contributor

jasonjkeller commented Sep 23, 2020

Yea, as @breedx-nr mentioned we were hoping to replace the old registry with the new one in this repo so we followed the same naming convention. Eventually we would like to archive this repo and replace the source here: https://github.com/micrometer-metrics/micrometer/tree/master/implementations/micrometer-registry-new-relic

I believe those efforts are currently held up by #46 which looks like it just went inactive. I'll see if we can get those efforts revived again. :)

I do agree though, it is a bit painful seeing classes in this project use the io.micrometer.newrelic package name while the artifact itself uses a different name for the groupId:

<dependency>
        <groupId>com.newrelic.telemetry</groupId>
        <artifactId>micrometer-registry-new-relic</artifactId>
        <version>0.5.0</version>
    </dependency>

@ebullient
Copy link
Contributor Author

Yea, as @breedx-nr mentioned we were hoping to replace the old registry with the new one in this repo so we followed the same naming convention. Eventually we would like to archive this repo and replace the source here: https://github.com/micrometer-metrics/micrometer/tree/master/implementations/micrometer-registry-new-relic

I believe those efforts are currently held up by #46 which looks like it just went inactive. I'll see if we can get those efforts revived again. :)

I do agree though, it is a bit painful seeing classes in this project use the io.micrometer.newrelic package name while the artifact itself uses a different name for the groupId:

<dependency>
        <groupId>com.newrelic.telemetry</groupId>
        <artifactId>micrometer-registry-new-relic</artifactId>
        <version>0.5.0</version>
    </dependency>

That is what flagged my interest to begin with. ;)

From an ongoing maintenance perspective, I think that's a decision you guys need to make. This is (obviously) my suggestion. If you switch to maintaining here, then this is what I would recommend re: naming (naming things is hard, so maybe I made a silly choice).

What I'll try next is some special sauce for Spring.. you'll want that shipped as a different maven artifact (do you want a different project or shift this around to produce two artifacts?)

@jasonjkeller
Copy link
Contributor

jasonjkeller commented Sep 24, 2020

@ebullient I think we'll likely go ahead with these changes and move all of the packages to com.newrelic.telemetry.micrometer. If/when we end up moving this repo to the micrometer Github org then we can change it back then. :)

Can you clarify the last bit about shipping two different artifacts? I'm not sure I've connected the dots on that yet. I assume we continue publishing this artifact as is and the second (new) artifact is something specifically related to the Spring auto-config special sauce?

Unless the codebase for the new artifact is a massive or reusable elsewhere I'd lean towards just making it a subproject of this repo.

@ebullient
Copy link
Contributor Author

Can you clarify the last bit about shipping two different artifacts? I'm not sure I've connected the dots on that yet. I assume we continue publishing this artifact as is and the second (new) artifact is something specifically related to the Spring auto-config special sauce?

Exactly. ;)

You could make this a single repo that creates/builds/publishes two maven artifacts, or you could use a second repo for the other artifact. In either case, the other artifact is the spring special sauce. ;)

Copy link
Contributor

@jasonjkeller jasonjkeller left a comment

Choose a reason for hiding this comment

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

These changes look good to me!

@jasonjkeller
Copy link
Contributor

@ebullient I guess if the spring special sauce is specific to this micrometer registry then let's proceed with a single repo that produces both artifacts.

I'll go ahead and merge this PR.

@jasonjkeller jasonjkeller merged commit 2f9234c into newrelic:main Sep 24, 2020
@ebullient ebullient deleted the rename branch September 24, 2020 18:31
@breedx-nr breedx-nr added this to In progress in Java Engineering Board Sep 28, 2020
@breedx-nr breedx-nr moved this from In progress to Done in Java Engineering Board Sep 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants