Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Merge custom resources on detection#515

Merged
c24t merged 6 commits intocensus-instrumentation:masterfrom
c24t:better-resource-detection
Mar 8, 2019
Merged

Merge custom resources on detection#515
c24t merged 6 commits intocensus-instrumentation:masterfrom
c24t:better-resource-detection

Conversation

@c24t
Copy link
Copy Markdown
Member

@c24t c24t commented Feb 22, 2019

This PR changes resource detection (monitored_resource.get_instance) to include custom type and resource labels if available, and to include labels from multiple resources if we detect more than one known environment (e.g. kubernetes on AWS).

Fixes #457 and #514.

cc @mayurkale22

Copy link
Copy Markdown
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

I suggest we fix #510 first, since it doesn't make much sense to combine gke_containter resource with AWS resource (while it makes sense for k8s_container to also be an EC2 instance).

Comment thread opencensus/common/resource.py Outdated

def merge_resources(r1, r2):
"""Merge two resources to get a new resource.
def merge_resources(resource_list):
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.

@c24t I'd like to hear about your opinion.

Shall we consider the signature of os.path.join? https://docs.python.org/2/library/os.path.html#os.path.join

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Part of the motivation for making the arg a list was to match the signature in java, but since *args in python is more idiomatic than varargs in java, maybe this is the better approach here, and I like this pattern as a user.

You think callers are more likely to have to make a list to call merge_resources than have to splat a list if we mimic os.path.join?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

I don't have strong opinion here, just putting another option and see which one you would prefer.
Please do what you think is better. Either way looks good to me :)

@c24t
Copy link
Copy Markdown
Member Author

c24t commented Feb 22, 2019

@songy23 good call, let's wait on this until after #510.

Copy link
Copy Markdown
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM, let's wait for #510.

@c24t c24t force-pushed the better-resource-detection branch from 5634048 to a31fdbb Compare March 8, 2019 22:27
@c24t c24t merged commit 913d588 into census-instrumentation:master Mar 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants