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 initial code #1

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Add initial code #1

merged 5 commits into from
Jan 17, 2024

Conversation

tannalynn
Copy link
Contributor

@tannalynn tannalynn commented Jan 5, 2024

@ioquatix
Copy link

ioquatix commented Jan 5, 2024

This looks great to me.

It's been hard to find a good intersection between different APMs. resource: is one area where I had some concerns. Without it, Datadog APM is substantially less good.

Did you have any feelings about the data model or the way it's exposed?

Ideally, we can find a nice consensus that allows every APM to expose a reasonable amount of information in a consistent way, such that software that uses traces doesn't need to adapt for specific APM backends.

It's a great opportunity now to receive that feedback so we can adapt the interface if there were any New Relic specific details that don't fit quite right with the existing interface, or areas you felt could be better modelled according to your own internal APM design.

In this regard, I think we have some room to evolve the design during the 0.x phase of traces/metrics gems. Once we nail it (and I've tried to keep the scope as simple as possible), we can release a 1.x across the gems.

One area which I'm aware of is header injection, or custom metadata.

Would it be strange to inject this into the trace context header and extract it at the remote end? IIUC, that's one of the purpose of that header.

@tannalynn
Copy link
Contributor Author

@ioquatix Thanks for taking a look! I really appreciate it.

This looks great to me.

It's been hard to find a good intersection between different APMs. resource: is one area where I had some concerns. Without it, Datadog APM is substantially less good.

Did you have any feelings about the data model or the way it's exposed?

I primarily tested with async http, and I found that I had access to all the information I expected to create spans with the same info the agent currently does. It's been a few weeks since I was really working on it due to the holidays (and then having to wait for a new repository to be made for me by new relic) so its not super fresh, but I can't think of anything that stood out as concerning re: the information that was available. I found that I didn't end up using resource, since everything we expected to use was either the name or available in attributes/the yielded attributes.

Ideally, we can find a nice consensus that allows every APM to expose a reasonable amount of information in a consistent way, such that software that uses traces doesn't need to adapt for specific APM backends.

It's a great opportunity now to receive that feedback so we can adapt the interface if there were any New Relic specific details that don't fit quite right with the existing interface, or areas you felt could be better modelled according to your own internal APM design.

In this regard, I think we have some room to evolve the design during the 0.x phase of traces/metrics gems. Once we nail it (and I've tried to keep the scope as simple as possible), we can release a 1.x across the gems.

One area which I'm aware of is header injection, or custom metadata.

At this time, I think that the header injection part is the only thing that new relic is really that different about as far as I can tell. For the most part, the otel implementation is fairly similar to what I've done for new relic. Just had to do some adjustments to account for some different ways our some of our objects/attributes work (like adding the attributes to spans using a hash instead of directly).

Would it be strange to inject this into the trace context header and extract it at the remote end? IIUC, that's one of the purpose of that header.

In theory that makes sense and I can totally see why you've suggested it, but unfortunately that wouldn't work for our situation.

Since these are headers we insert into external calls, changing the way it works would cause a lot of issues with backwards compatibility with the external service since they could be running older ruby agent versions, or using a different language agent. Since all the other new relic language agents need to be able to parse the header info, getting it updating across all languages and changing the agent specs that all new relic agents have to comply with would be pretty involved and even after that, there still wouldn't be compatibility with any versions of the agents that are out there already.

I totally understand not wanting to build in something to accommodate that since its a pretty specific ask, so I think a fair alternative is recommending people who expect to be using features that require those headers (the main thing being the new relic synthetics product) to use the current in agent instrumentation instead, since it can handle all of it. Not everyone uses those features, so the majority of people could just use traces. We will be adding a check in the agents instrumentation installation so that it will not be installed if this is also installed, so there won't be any conflicts there.

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Cool work, Tanna! Thanks for building this!

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/traces/backend/newrelic/version.rb Outdated Show resolved Hide resolved
traces-backend-newrelic.gemspec Show resolved Hide resolved
README.md Show resolved Hide resolved
traces-backend-newrelic.gemspec Show resolved Hide resolved
tannalynn and others added 4 commits January 16, 2024 13:57
Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
@tannalynn tannalynn merged commit 83a9558 into main Jan 17, 2024
1 check passed
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.

4 participants