Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Add resource to protocol #137

Merged
merged 1 commit into from
Nov 2, 2018
Merged

Add resource to protocol #137

merged 1 commit into from
Nov 2, 2018

Conversation

fabxc
Copy link
Contributor

@fabxc fabxc commented Oct 12, 2018

No description provided.


// The resource for the metrics in this message. If unset, the resource of the
// most recent node applies.
opencensus.proto.agent.common.v1.Resource resource = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bogdandrutu this allows to overwrite the default resource for the batch of metrics in this request.

Here I've been wondering, whether it would make sense to remove the resource field from node, and just give this resource field here the same semantics as the node field, i.e. the most recent set value applies.
It removes one layer of nesting and gives both fields the same defaulting semantics, which seems to make semantics a bit simpler.

Just an alternative though – nothing fundamentally superior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should Resource be part of metrics instead of just metrics_service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean out of the agent package and into the top level metrics package?
Given resource will not only apply to metrics but also spans (and potentially other signals in the future), it would rather need its own top-level package then.

I thought about that but then discarded the idea since it seems we can avoid attaching it to the signal-specific messages entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given resource will not only apply to metrics but also spans (and potentially other signals in the future), it would rather need its own top-level package then.

I agree. I'm just wondering if there are any other sources (other than OC-Agent, for example library exporters) that expect resource along with metrics. With the current approach, resource is only available to OC-Agent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right I think. Talked to Bogdan about this earlier, and giving signals an explicit resource, which may be empty in many cases and defaulted to what is provided in the agent protocol stream seems better.

@bogdandrutu
Copy link
Contributor

Please let me know when this is ready for a new review.

Signed-off-by: Fabian Reinartz <freinartz@google.com>
@fabxc
Copy link
Contributor Author

fabxc commented Oct 15, 2018

@bogdandrutu PTAL. I did not add an explicit resource to spans yet, since we didn't have a use case for that yet AFAIK. Can be added later without much hassle I think.
I pulled resource out of Node as proposed in my inline comment.

// If unset, the most recently set resource in the RPC stream applies. It is
// valid to never be set within a stream, e.g. when no resource info is known
// at all or when all sent metrics have an explicit resource set.
opencensus.proto.resource.v1.Resource resource = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

One intention for these protocol is to be used for example when collect data from multiple nodes on the same RPC stream:
{App1, App2} -> oc-agent1 -> oc-agent2

Let's say oc-agent1 receives:

  • From App1 {Node1, Metrics1, Resource1}
  • From App2 {Node2, Metrics2, null (no resource)}

Then oc-agent1 sends to oc-agent2:

  • {Node1, Metrics1, Resource1}
  • {Node2, Metrics2, null (no resource)}

Based on the current description Metrics2 will be Resource1 which is not correct.

Couple of suggestions:

  1. If no-resource then we should send an empty Resource. or type = unknown.
  2. Resources are cached per Node. So I would say "If unset, the most recently set resources in the RPC stream for the same Node ..."

Copy link
Contributor Author

@fabxc fabxc Oct 17, 2018

Choose a reason for hiding this comment

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

Agreeing with 1) – empty resource seems fine. I can't see any case where we'd want to distinguish an unset and an empty resource.

Suggestion 2. concerns me a bit, e.g. in particular this case:

  • 1) app1 -> {node1, metrics1, resource1}
  • 2) app2 -> {node2, metrics2, resource2}
  • 3..999) without app1 appearing
  • 1000) app1 -> {node1, metrics3, empty/unset}

It seems awkward to jump back to using resource1 in step 1000. For long streams with lots of senders, this could become rather hard to reason about during debugging. It also requires the cache of a single connection to grow linearly with the number of apps multiplexed over it.
That's probably not a problem in most use cases, but certainly an easy way to DDoS the agent.

Would it make sense to slightly alter this to: "A change in Node always resets the most recent Resource"?

Effectively, changing the node requires the resource to be changed too (or be considered empty after). This way, the node and resource cache of a connection have a constant size of 1 as well.

@songy23
Copy link
Contributor

songy23 commented Oct 22, 2018

Friendly ping.

@fabxc fabxc changed the title [WIP] Add resource to protocol Add resource to protocol Oct 23, 2018
@fabxc
Copy link
Contributor Author

fabxc commented Oct 26, 2018

@bogdandrutu regarding distinguishing between empty and unset resources, should we then just go with a sentinel type opencensus.io/unset which is only used in the agent protocol?

@bogdandrutu
Copy link
Contributor

@fabxc I agree with that. To make sure everyone understands the concern see the example:

Export {
Resource = resource1;
Metrics = [metric1, metric2, metric3];
}

metric1 = {
resource = null;  // empty to use the resource1
}

metric2 = {
resource = resource2;  // It's own resource
}

metric3 = {
resource = unset;  // avoid using the resource1 but cannot determine the resource
}

If we cannot distinguished between unset and null then metric3 will have resource1.

@bogdandrutu
Copy link
Contributor

Maybe instead of unset call it "unknown"?

@songy23
Copy link
Contributor

songy23 commented Nov 2, 2018

Filed #146 to follow up with the left comments. Going to merge this PR now and making a new release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants