-
Notifications
You must be signed in to change notification settings - Fork 587
feat(gcp-detector): add Cloud Run support with faas.* and cloud.platform #2818
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
base: main
Are you sure you want to change the base?
Conversation
* feat(gcp-detector): add Cloud Run support with faas.name, faas.version, and faas.instance Signed-off-by: Kasper Borg Nissen <[email protected]> * chore(lint): remove newlines Signed-off-by: Kasper Borg Nissen <[email protected]> * chore(comment): write comment on moving the assertions * test(gcp-detector): fix Cloud Run tests --------- Signed-off-by: Kasper Borg Nissen <[email protected]>
#2) Signed-off-by: Kasper Borg Nissen <[email protected]>
I am looking forward to seeing this merged :-) |
I am no longer working in this area, @aabmass could you please take a look, and also update component owners? |
@aabmass could you have a look at this one? I could volunteer to review it but I think you feedback would be better |
Hey, apologies for the confusion. We have already implemented this in I would actually like to make the resource detector here in contrib just re-expose the one from I previously discussed this with the maintainers and it wasn't contentious but haven't found the time to do it. To recap, the reason to use it as a library vs copying the code is that we have a bunch of integration tests that would be really difficult to move to this repo. |
Thanks for the update and for clarifying the intent to centralize this logic in the That said, I’d like to raise a few points for consideration - especially as someone who's still relatively new to the inner workings of this project. From the outside, it feels a bit problematic that Cloud Run support isn't discoverable or available out-of-the-box when using the standard auto-instrumentation entry point (e.g., I’m also trying to better understand whether the broader architectural goal for this repo is to become a collection of thin wrappers around vendor-specific detectors, or whether upstream detectors are still expected to house meaningful logic directly. If the former, what’s the long-term vision for user experience and cross-SDK consistency? For example, the Go SDK includes Cloud Run detection directly. I don’t mean to push back unnecessarily - just trying to better understand the direction and see if there’s a path forward that balances reuse with user experience and visibility. Would love to hear your thoughts on how we might reconcile these concerns. |
We actually talked about this at the JS SIG meeting last week. We really don't want to be in the situation where we're publishing thin wrappers around code we don't control or understand. At the same time, we don't really have the bandwidth to maintain instrumentations for every possible module, service, and app. It is much easier for us as maintainers if these are externally hosted. Ideally, we would be able to load these automatically with the SDK if the user installs them, but right now there is no such mechanism for that automation. It's been something we've talked about periodically though. Some things like ESM complicate this story even further. The other advantage of having things in this repo is discoverability. There is a registry to solve this problem, but I think it isn't particularly well used or known by users. |
Thank you, @dyladan for the explanation. I saw in the meeting notes that this PR was actually discussed 🙈 I’ll try to start joining the JS SIG meetings to stay more in the loop. I totally get that this is a tradeoff - and I understand the concern about maintaining wrappers for code the SIG doesn’t directly own. At the same time, it does feel like we’re missing out on visibility and adoption when commonly expected detectors (like for Cloud Run) aren’t available through the default experience, especially in something like @opentelemetry/auto-instrumentations-node. I also checked the registry, but I don’t see the GCP resource detector listed there - which might contribute to the discoverability issue for new users. I’d be happy to try and help push this forward in some way - though, like everyone else, I also have limited bandwidth (and knowledge in this area). But if there’s something small I can do to help improve the situation, I’m definitely open to it. Thanks again for the context - and for all the work you’re doing on this! |
I think at a minimum you should add your detector to the registry. We're discussing ways to make a more automated experience for non-otel-hosted packages, but so far nothing is implemented. I'll try to keep you updated but I don't have a lot more on that right now. For now, it's up to you if you want to maintain both packages, but I don't think we're likely to accept a thin wrapper in this repo without further discussion. On a somewhat related note, this package currently supports SDK 2.0 and it looks like the GCP-hosted one only supports SDK 1.0 |
Thanks for the discussion. I can try to join next Wednesday if you'd like to discuss again, or I'll just pay closer attention here. I agree there are pro/cons to each approach. Unfortunately GCP resource detection is just kind of complicated–we would either depend on
If you look closely, the GCP detector for Go does something pretty similar (Java too) but the implementation of the actual interface is still in the contrib repo. Daniel, would a similar level of logic split between a GCP repo and here be reasonable to you? |
Quick update, I would be happy to just move the resource detector code we have in the Google repo directly into this package. We can do integration tests out of tree. @dyladan does that sound OK to you? |
I almost feel like this needs input from GC/TC or something. I wish there was a way we could solve the discoverability and automatic setup for external packages. Might have to finally start eating that elephant. |
We discussed in the JS SIG on Wed July 23rd. The decision was to move the Google repo code into this package. I'll send a PR and we can close this one at that point. I will also take a shot at avoiding the |
Which problem is this PR solving?
The resource detector for gcp did not previously support Cloud Run–specific resource attributes, although these are supported in other SDKs (e.g. Go, Java). This PR adds support for setting the
faas.*
along withcloud.platform
resource attributes when running in a Cloud Run environment.Short description of the changes
faas.name
fromK_SERVICE
environment variable.faas.version
fromK_REVISION
environment variablefaas.instance
from GCP metadata server (/instance/id
) reusing the existing method.cloud.platform
using the semantic conventions static variables.For more information, see the container runtime contract: https://cloud.google.com/run/docs/container-contract#env-vars.
This is my first PR 🙏
I've added another PR for the extension of the
assertCloudResource
helper withcloud.platform
. For now just manually asserting in the test case.