Remote configuration for the SDK proposal #121

churanchen · 2020-06-25T17:56:48Z

This proposal asks for approval to create an experimental feature for remote configuration of the SDK, per [this] (https://github.com/open-telemetry/opentelemetry-specification/tree/master/experimental) framework. This experiment specifically focuses on configuring metric collection.

It is related to this PR: open-telemetry/opentelemetry-proto#155

Spec: OpenTelemetry.Configuration.Service.pdf

Co-authored-by: Sergey Kanzhelev <[email protected]>

linux-foundation-easycla · 2020-06-25T17:56:54Z

The committers are authorized under a signed CLA.

✅ Churan Chen (ed7877e, 63cbe99, 51a6f1d, 27d3b47, 9901e1e, dd2a31e, 5ec717f, 83c9b96, baeb1cd, 8b4e615, 4a03244, c2e6ac1)

text/0121-config-service.md

SergeyKanzhelev · 2020-06-30T21:31:14Z

@tigrannajaryan thank you for review! Very good topics are raised. Do you agree with this project as an experimental project to be moved forward?

It is hidden in the otep description, but the motivation is to get this to POC and than decide on how to move it to the main product:

It is intended to seek approval to develop a proof of concept.

I think this POC may uncover some limitations of current SDKs config story.

SergeyKanzhelev

approving as an experiment with the couple suggestions to address tigran's questions

tigrannajaryan · 2020-06-30T21:45:40Z

@tigrannajaryan thank you for review! Very good topics are raised. Do you agree with this project as an experimental project to be moved forward?

It is hidden in the otep description, but the motivation is to get this to POC and than decide on how to move it to the main product:

It is intended to seek approval to develop a proof of concept.

I think this POC may uncover some limitations of current SDKs config story.

Yes, definitely, I think remote configuration is a very useful addition. Assuming this is an experiment I do not see anything that prevents us from moving forward. For production use this will need more refinement but it can be done after the PoC.

c24t

A few nits and questions, but LGTM to go ahead. Looking forward to using this to configure tracing too!

text/0121-config-service.md

c24t · 2020-06-30T21:58:37Z

text/0121-config-service.md

+```
+message ConfigResponse {
+
+  // Optional. The fingerprint associated with this ConfigResponse. Each change


Should response fields be optional?

Our intention for the fingerprint field was to have an identifier for configs that the user can set and use if they wish, similar to the metadata field. The idea being that there might be some fields that are nice and useful to include, but that we shouldn't force the end users to have in their configs.

text/0121-config-service.md

yurishkuro · 2020-07-02T22:24:48Z

I am bothered by the wording in the motivation:

This OTEP is a request for an experimental feature... It is intended to seek approval to develop a proof of concept.

OTEL is supposed to be a collaborative project, not a rigid process. No "approval" is required to develop experimental features or proofs of concept.

OTEP is an "enhancement proposal". You're proposing new functionality and the OTEP is used to discuss the design decisions and pros & cons, not to seek approval.

It is also a good practice to cite prior art. This is not a new idea to have a remotely controlled configuration, Jaeger SDKs have implemented it from day 1.

yurishkuro · 2020-07-13T21:39:29Z

text/0121-config-service.md

+      // For this iteration, since we only want one Schedule that applies to all metrics,
+      // we will not check the inclusion_patterns and exclusion_patterns.
+      repeated Pattern inclusion_patterns = 1;
+      repeated Pattern exclusion_patterns = 2;


nits:

because exclusion patterns have higher priority per the comment above, I would move it to the first position.

I am not sure this grammar is expressive enough. Is there any prior art for this?

Changed!

The choice to use exact match and starts with was made because they are more efficient to apply and clearer to read than more expressive alternatives, and have in practice been the only required pattern types for specifying metric names in Google's internal metric collection configuration protocol. However we are open to also adding a more expressive grammar such as regex or glob as a third option for a pattern type, so that monitoring backends can optionally choose to support a more expressive grammar without affecting efficiency for the simpler pattern types.

Co-authored-by: Yuri Shkuro <[email protected]>

…teps into remote-configuration

churanchen · 2020-07-20T18:54:14Z

Would welcome any more reviews!

jmacd · 2020-07-21T03:22:02Z

@churanchen Could you resolve the issues that you feel are resolve (and/or indicate which are resolved)? This helps reviewers.

bogdandrutu · 2020-07-21T17:56:40Z

@yurishkuro is this ready to be merged in your opinion?

@SergeyKanzhelev should we have a way to mark that this is experimental vs not?

yurishkuro · 2020-07-22T01:35:22Z

I have no objections to merging. I have not had experience with metrics library that requires external configuration, so aside from the general config mechanism I don't have an opinion on the metrics-specific details of this proposal.

bogdandrutu · 2020-07-22T15:32:53Z

text/0121-config-service.md

@@ -0,0 +1,127 @@
+# A Dynamic Configuration Service for the SDK


Consider to move this to experimental/... this way we know that it is experimental.

Done! @SergeyKanzhelev, would it be a good idea to move the z-pages otep there as well?

…teps into remote-configuration

* Otep for dynamic configuration * Revisions * Further explanation * Fix link * Revise proposal Co-authored-by: Sergey Kanzhelev <[email protected]> * Revisions * Make experimental * Slight revisions * Revisions * Phase 1 * Revisions * Change filename * Fix lint errors * Update text/0121-config-service.md Co-authored-by: Chris Kleinknecht <[email protected]> * Revisions * Fix lint errors * Add metadata explanation * Update text/0121-config-service.md Co-authored-by: Yuri Shkuro <[email protected]> * Update text/0121-config-service.md Co-authored-by: Yuri Shkuro <[email protected]> * Update text/0121-config-service.md Co-authored-by: Yuri Shkuro <[email protected]> * Update text/0121-config-service.md Co-authored-by: Yuri Shkuro <[email protected]> * Change period from enum to int * Specified service for metrics, removed tracing * Switch to MetricConfig * Clarify period description * Remove metadata field * Move to experimental folder Co-authored-by: Sergey Kanzhelev <[email protected]> Co-authored-by: Chris Kleinknecht <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Bogdan Drutu <[email protected]>

churanc and others added 12 commits June 10, 2020 12:40

Otep for dynamic configuration

ed7877e

Revisions

63cbe99

Further explanation

51a6f1d

Fix link

27d3b47

Revise proposal

9901e1e

Co-authored-by: Sergey Kanzhelev <[email protected]>

Revisions

dd2a31e

Make experimental

5ec717f

Slight revisions

83c9b96

Revisions

baeb1cd

Phase 1

8b4e615

Revisions

4a03244

Change filename

c2e6ac1

churanchen requested review from bogdandrutu, carlosalberto, jmacd, reyang, SergeyKanzhelev, tedsuo, tigrannajaryan and yurishkuro as code owners June 25, 2020 17:56

Fix lint errors

72569b2

tigrannajaryan reviewed Jun 30, 2020

View reviewed changes

SergeyKanzhelev approved these changes Jun 30, 2020

View reviewed changes

c24t approved these changes Jun 30, 2020

View reviewed changes

bogdandrutu approved these changes Jul 2, 2020

View reviewed changes

tigrannajaryan approved these changes Jul 6, 2020

View reviewed changes

yurishkuro reviewed Jul 13, 2020

View reviewed changes

churanchen and others added 11 commits July 13, 2020 21:53

Update text/0121-config-service.md

911aa42

Co-authored-by: Yuri Shkuro <[email protected]>

Update text/0121-config-service.md

265debc

Co-authored-by: Yuri Shkuro <[email protected]>

Update text/0121-config-service.md

f9d24e7

Co-authored-by: Yuri Shkuro <[email protected]>

Update text/0121-config-service.md

181ec16

Co-authored-by: Yuri Shkuro <[email protected]>

Change period from enum to int

2b976c9

Specified service for metrics, removed tracing

bf284b7

Switch to MetricConfig

1ecf1c9

Clarify period description

e310cd8

Merge branch 'master' into remote-configuration

45195d8

Remove metadata field

31ef3ea

Merge branch 'remote-configuration' of https://github.com/vmingchen/o…

3cb8013

…teps into remote-configuration

jmacd mentioned this pull request Jul 18, 2020

Replace Temporality with Kind; Type with ValueType open-telemetry/opentelemetry-proto#168

Closed

jmacd approved these changes Jul 21, 2020

View reviewed changes

Merge branch 'master' into remote-configuration

b4c8684

bogdandrutu approved these changes Jul 22, 2020

View reviewed changes

churanc added 2 commits July 22, 2020 13:00

Move to experimental folder

142a718

Merge branch 'remote-configuration' of https://github.com/vmingchen/o…

1a0dbb3

…teps into remote-configuration

SergeyKanzhelev merged commit efa4534 into open-telemetry:master Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote configuration for the SDK proposal #121

Remote configuration for the SDK proposal #121

churanchen commented Jun 25, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 25, 2020 •

edited

Loading

SergeyKanzhelev commented Jun 30, 2020 •

edited

Loading

SergeyKanzhelev left a comment

tigrannajaryan commented Jun 30, 2020 •

edited

Loading

c24t left a comment

c24t Jun 30, 2020

churanchen Jul 6, 2020

yurishkuro commented Jul 2, 2020

yurishkuro Jul 13, 2020

churanchen Jul 14, 2020

churanchen commented Jul 20, 2020

jmacd commented Jul 21, 2020

bogdandrutu commented Jul 21, 2020

yurishkuro commented Jul 22, 2020

bogdandrutu Jul 22, 2020

churanchen Jul 22, 2020

		@@ -0,0 +1,127 @@
		# A Dynamic Configuration Service for the SDK

Remote configuration for the SDK proposal #121

Remote configuration for the SDK proposal #121

Conversation

churanchen commented Jun 25, 2020 • edited Loading

linux-foundation-easycla bot commented Jun 25, 2020 • edited Loading

SergeyKanzhelev commented Jun 30, 2020 • edited Loading

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Jun 30, 2020 • edited Loading

c24t left a comment

Choose a reason for hiding this comment

c24t Jun 30, 2020

Choose a reason for hiding this comment

churanchen Jul 6, 2020

Choose a reason for hiding this comment

yurishkuro commented Jul 2, 2020

yurishkuro Jul 13, 2020

Choose a reason for hiding this comment

churanchen Jul 14, 2020

Choose a reason for hiding this comment

churanchen commented Jul 20, 2020

jmacd commented Jul 21, 2020

bogdandrutu commented Jul 21, 2020

yurishkuro commented Jul 22, 2020

bogdandrutu Jul 22, 2020

Choose a reason for hiding this comment

churanchen Jul 22, 2020

Choose a reason for hiding this comment

churanchen commented Jun 25, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 25, 2020 •

edited

Loading

SergeyKanzhelev commented Jun 30, 2020 •

edited

Loading

tigrannajaryan commented Jun 30, 2020 •

edited

Loading