Allow users to "unset" SDK limits and evaluate default limits lazily instead of on import #1839

owais · 2021-05-11T01:43:12Z

Description

Added SpanLimits class
TracerProvider now optionally accepts an instance of SpanLimits which is passed all the way down to Spans.
Spans use the limits to created bounded lists or dicts for different fields.

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added tests

Does This PR Require a Contrib Repo Change?

Yes. - Link to PR:
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

owais · 2021-05-11T01:44:40Z

@lzchen this PR implements what we discussed here

owais · 2021-05-11T02:10:54Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+    max_attributes=SpanLimits.UNSET,
+)
+
+SPAN_ATTRIBUTE_COUNT_LIMIT = SpanLimits._value_or_default(


This is not being used anywhere anymore but removing it would break the Python interface of this module. This is strictly a read only variable as over-writing it has no effect at all in the current implementation. I'm not sure what to do with this without completely re-writing this whole module as a class or override class on module which doesn't work with 3.6.

https://stackoverflow.com/questions/922550/how-to-mark-a-global-as-deprecated-in-python

lzchen · 2021-05-11T16:28:28Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -564,6 +562,79 @@ def _format_links(links):
        return f_links


+class SpanLimits:


Is there a way to accomplish this without introducing a new class? It adds more surface area to our api/sdk and also users would have to import/use this class when trying to configure the TracerProvider.

We could add every individual limit to the tracer provider constructor but as the number of limits we support grows, it'll get very ugly fast. There are already two more limits suggested but not required by the spec with one more in the proposal stage. That increases total limit to six. I feel a new class for an entire set of features is not that bad (Java does the same FWIW).

Alternatively, we can keep the class private and have a public function that return an instance of the private class but not sure how much better adding a public function is over adding a public class.

I don't understand why use a class here. If the limits are to be set when creating a tracer provider, can't the limits themselves be tracer provider arguments? How does having a class will make it less ugly to support more limits? Also, everything is a class in Java, right?

Without bundling these variables and the logic around them, it'll all bleed into the tracer provider (I don't think it belongs there). It also means provider has to pass all individual limits as different variables down the chain meaning provider, tracer, span and internal functions all receive these 3 integers as arguments which will grow to 6 soon and in future could grow even further.

Provider(max_events_limit, max_attributes_limit, max_links_limit) max_events_limit, max_attributes_limit, max_links_limit = _process_limit_defaults(max_events_limit, max_attributes_limit, max_links_limit) Resource(max_attributes_limit) Tracer(max_events_limit, max_attributes_limit, max_links_limit) Span(max_events_limit, max_attributes_limit, max_links_limit) _process_links(max_links_limit) _enforce_attribute(max_attributes_limit) // other functions that may pass these down the chain

The above seems uglies to me compared to the following but I acknowledge that it is a big subjective and comes down to personal taste to some extent.

Provider(limits: SpanLimits) Resource(limits) Tracer(limits) Span(limits) _process_links(limits.max_links) _enforce_attribute(limits.max_attributes) // other functions that may pass these down the chain

Note that we only support 3 of 5 limits specified by spec right now and one more will be added soon. So that increases the number of arguments to 6 in near future.

I feel like this is self-contained enough that provider shouldn't care about the logic to process these limits and instead just consume them. To me this looks more like a Sampler or IdGenerator to me. We could pass raw sampling decision or id generation functions for trace/span ID as individual arguments to provider but they are logically bound together that it makes sense to bundle them in a single type. A class also gives us a bit more flexibility in the long run. For example, limits can be dynamically updated by a remote configutation without having to re-create providers/tracers.

That said, what we need today work by passing each limit as an argument to provider/tracer/span. So happy to change if others feel too strongly about it.

BTW how about if we rename it to just Limits instead of SpanLimits? That should make it a bit more future proof as it'll allow us to use same thing for metrics and later logging as well.

On the class increasing API surface.

The class does indeed increase the API surface but it is relatively harmless IMO given how the class does not affect anything on it's own. It only reads values passed to it as arguments, read values from the environment and sets some variables. This means if for some reason we don't want to use this in future, we can just leave the class as is without it affecting anything else.

On the other hand, adding new arguments to tracer provider is a much bigger deal IMO. Even if don't add a new class to the API, we'd still be adding a bunch of arguments to the tracer provider constructor. I think that is probably something much more important get right.

Alternative

One thing we could do is to neither add any new public classes, nor change the provider constructor signature. Instead we rename the class to _SpanLimits and add a private method on provider so it can be used like:

provider = TracerProvider() # internally sets self._limits to default _Limits provider._set_limits(_Limits(max_events=10)) # provider = TracerProvider()._with_limits(_Limits(max_events=10))

This allows us to solve the issue with lazy evaluation of limits while not increasing the API surface. Distros can still us the private methods to set custom limits. This will buy us time to experiment with the design and make it public in future if desired.

One downside is that this still doesn't allow end users to set limits via code but I think that's fine and can be solved later.

Setting limits via code is a "Feature" that we chose to implement correct? What do you think about just having a kwargs in the traceprovider constructor?

Also, I understand the messiness of having to pass 3-5 variables down the call chain. Can we pass in variables through the constructor, but use _SpanLimit internally maybe?

I wanted to bring this up in the SIG meeting yesterday but couldn't join.

Yes, we can do that but the question still remains whether we prefer a single limits kwargs vs 3-6 and possible an ever growing list of limit kwargs in tracer provider. It sounds like you and @ocelotl at least prefer individual kwargs over an encapsulated single kwarg.

We can go a step further for this PR and not add any new attributes to tracer provider constructor at all. Internall tracer provider will construct an instance of _SpanLimit and pass it down the chain. That way we will have lazy evaluation in this PR while we discuss what doing this in code should look like. May be we can live with just env var based config for a while and buy some time to think about the Python interface for this especially as the metrics API stablizes.

I see how putting all these limits inside an object seems more convenient, but I don't see any real difference between passing the limits in an object that got them previously or in passing them directly as arguments. kwargs should not be used for this purpose because these arguments are known beforehand.

A couple of questions. Span Limits look like Span attributes. Why are we passing them to a resource, tracerprovider, tracer, etc? This feels like they should just be added in the Span. If we add these span limits to the arguments a tracer provider receives, why don't we also pass the rest of the arguments the Span receives to the tracer provider?

Maybe we can just add them to the Span arguments. If that is the case, we could just use take their values from environment variables as usual.

I've removed ability to configure limits via code to simply the PR (#1839 (comment)) but to answer your questions:

Span Limits look like Span attributes. Why are we passing them to a resource, tracerprovider, tracer, etc?

You're right. It's a bad name. I've renamed it to _Limits. These will be used with resources very soon and at some point with metrics.

If we add these span limits to the arguments a tracer provider receives, why don't we also pass the rest of the arguments the Span receives to the tracer provider?

Because the feature was implement "limit configuration with code". There is no way for users to pass something to every single Span's contructor. It can only be done with manually created spans and even then usually users want limits to be configured once or once per provider (different limits for manual vs instrumentation spans or different limits for different instrumentation), not once per span.

We could have something like _set_global_limits() similar to global provider and propagator but that wouldn't allow limits to be set per instrumentation or different limits for different components of a service which would be really great to have. In order to make this happen, IMO, the best place to configure limits is TracerProvider (and in future MeterProvider) constructor.

That was the rationale but I've removed it all for now in order to have no Python API changes. I'm still creating a single _Limits instance per provider to not run the code over and over again for every single span or tracer and I'll be passing it to resources in an upcoming PR (again without public API changes).

Let's discuss this during the SIG meeting. Please add this to the list of topics.

ocelotl · 2021-05-13T04:14:04Z

CHANGELOG.md

@@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  ([#1823](https://github.com/open-telemetry/opentelemetry-python/pull/1823))
 - Added support for OTEL_SERVICE_NAME.
  ([#1829](https://github.com/open-telemetry/opentelemetry-python/pull/1829))
+- Lazily configure Span limits and allow limits limits to be configured via code.


Suggested change

- Lazily configure Span limits and allow limits limits to be configured via code.

- Lazily configure Span limits and allow limits to be configured via code.

ocelotl · 2021-05-13T04:23:29Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -564,6 +562,79 @@ def _format_links(links):
        return f_links


+class SpanLimits:


I don't understand why use a class here. If the limits are to be set when creating a tracer provider, can't the limits themselves be tracer provider arguments? How does having a class will make it less ugly to support more limits? Also, everything is a class in Java, right?

owais · 2021-05-15T00:35:13Z

I've removed the ability to configure limits via code to unblock the PR. This PR now only implement the following:

configure limits (read from env) lazily instead of on module import.
allow users to set a limit to "unset".

This eliminates the public API changes and should unblock the PR. Any internal API changes should not be a big issue as we can change them anytime we want.

I'll build on top of this in #1847

ocelotl

Left a blocking comment. I still don't find convenient to have a wrapper class that just hides object attributes that legitimately belong to the object. If an object has now more attributes, that can be passed to it in its constructor, they should be treated normally as they have been before. That increasing the signature of the constructor method is not an issue, it is expected. There is no limit to the amount of arguments a function is able to receive and wrapping them in a class just adds another layer and more code.

ocelotl · 2021-05-17T17:30:47Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -604,6 +677,7 @@ def __init__(
        links: Sequence[trace_api.Link] = (),
        kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
        span_processor: SpanProcessor = SpanProcessor(),
+        limits: _Limits = _UnsetLimits,


I understand that you are trying to make limits private with _Limits, but I think exposing it here as an attribute type and default argument defeats the intended purpose. This is putting a private object in a public place.

Isn't Span an internal interface though? Otel API forbids anyone from creating spans directly so I don't see this as a public change but happy to change it if there are other use cases where we treat Span as part of public SDK interface.

owais · 2021-05-17T18:32:21Z

I still don't find convenient to have a wrapper class that just hides object attributes that legitimately belong to the object. If an object has now more attributes, that can be passed to it in its constructor, they should be treated normally as they have been before.

Yea, I do agree with you. It's just that I don't feel they belong to span/tracer/provider/resource per-se. To me the limits look like more like sampler or id_generator than something like shutdown_on_exit. May be calling it a limiter would help :) I realize the implementation is simplistic but I still feel they belong together logically. It is easy to imagine additional logical functionality being added to them like changing limits dynamically based on some remote config source.

That said, , I'm much more open to not having the class if that's the consensus since this is a private interface now.

owais · 2021-05-18T00:42:25Z

@lzchen @ocelotl I can change this to not add a new kwarg to Span constructor. Span is not supposed to be used directly so I think it's constructor should not be consider part of the public API but happy to change it if that's not correct.

@ocelotl if you feel too strongly about the class, I can pass in individual limits down the chain for now. Given that this does not touch the public API now, we can easily change things later if we want to.

I think these are the only two issues left. LMK what you think.

owais · 2021-05-18T23:55:00Z

@ocelotl I moved the limits arg from Span to _Span which is what we use internally anyway. This resolves the technical issue with Span being part of public API according to Python convention. LMK if this is good enough. This resolves your blocking comment.

On the limits class vs individual args, I feel class is a better implementation but I can totally change it if you feel strongly about it. LMK if you'd like me to replace it.

ocelotl

Yes, almost there. Minor implementation changes requested. My major objection (exposing a private attribute in a public API) has been fixed. 👍

ocelotl · 2021-05-19T00:04:21Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+    def __init__(self, *args, **kwargs):
+        self._limits: _Limits = kwargs.pop("limits", _UnsetLimits)


Suggested change

def __init__(self, *args, **kwargs):

self._limits: _Limits = kwargs.pop("limits", _UnsetLimits)

def __init__(self, *args, limits=_UnsetLimits, **kwargs):

self._limits: _Limits = limits

ocelotl · 2021-05-19T00:05:58Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -853,6 +926,11 @@ def __init__(
        self.span_processor = span_processor
        self.id_generator = id_generator
        self.instrumentation_info = instrumentation_info
+        self._limits = None
+
+    def _with_limits(self, limits: _Limits) -> "Tracer":


What is this for? We can just internally set the _limits attribute, right?

Yes, provider can set _limits on tracer after creating one but this reads better to me as it can be chained and tracer's internal structure/logic does not leak into provider. Happy to change though.

ocelotl · 2021-05-19T00:07:22Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+        )
+
+    @classmethod
+    def _create(cls):


Why not add this code in __init__?

Because None is a valid value for limits which means no limit has been set. If we do this in init, there is not way to know if None was explicily passed by the user to not set a limit or if the value was not provided at all in which case it should be loaded from env with fallback on default value.

This is more of a remnant from the earlier version which allowed users to set limits via code. In that version I also had a constant called limits.UNSET which could be passed to not set a value but since we removed the ability to set limits via code, this is probably not needed anymore. We can do two things here for now:

move this logic to init and add a new _create_empty() method that returns limits with all values unset. This will be used by the default "unset limits" instance that is used as the default value for spans. It feels a bit less flexible overall as it means either use _Limits() and always load default for missing kwarg or set all limits to unset by calling _Limits._crate_empty().

second option is to add the UNSET constant back. That way we can merge _create() into __init__ and allow setting any individual limit to UNSET as _Limits(max_events=_Limits.UNSET, max_links=100). In this example, there will be no upper limit on events, links will be limits to 100 and any other limits will be loaded from env or default values. I like this the most from an API point of the view but since this is not supposed to be used by users, I tried to simplify it by removing it. If you prefer this option, I can add it back.

Implemented option 2. and got rid of the _create() method.

ocelotl · 2021-05-19T00:10:46Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+            ),
+        )
+
+    @classmethod


This should not be a classmethod (because its code does not use cls) but a staticmethod.

codeboten · 2021-05-20T16:53:35Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+            str_value = environ.get(env_var, "").strip().lower()
+            if not str_value:
+                return default
+            if str_value == "unset":


should this be a constant?

Just checking is there an issue w/ the spec around using unset to disable limits?

I don't think so. There is precedent for using the value "unset" in other places but the limits section does not mention it in any way.

I can remove it for now so there will be no way to unset from an env var other than setting the limit to an very large value. Once we add support to set limits via code, it'll allow users to truly "unset" the limits.

codeboten

Code looks good! Just a question about whether we need an issue in the spec around the unset variable

codeboten · 2021-05-20T16:54:04Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+            str_value = environ.get(env_var, "").strip().lower()
+            if not str_value:
+                return default
+            if str_value == "unset":


Just checking is there an issue w/ the spec around using unset to disable limits?

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

lzchen · 2021-05-20T21:43:08Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+    max_links=_Limits.UNSET,
+)
+
+SPAN_ATTRIBUTE_COUNT_LIMIT = _Limits._from_env_if_absent(


Is this only here because it was exposed as an API attribute? Can we put a comment here saying that?

lzchen · 2021-05-20T21:55:33Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -853,6 +932,11 @@ def __init__(
        self.span_processor = span_processor
        self.id_generator = id_generator
        self.instrumentation_info = instrumentation_info
+        self._limits = None
+
+    def _with_limits(self, limits: _Limits) -> "Tracer":


So we're preventing from modifying the constructor of Tracer by using _with_limits, and the constructor of Span by using our private implementation of _Span? And in the future, if we decide on manual configuration of limits, a new parameter into TracerProvider?

If we are officially supporting setting limits manually as a feature of OT Python, shouldn't we do things consistently by just modifying the constructors (and our api surface) and simply pass in the parameter down the chain to Span? Are we hesitant on doing this because it's not defined in the specs?

So we're preventing from modifying the constructor of Tracer by using _with_limits, and the constructor of Span by using our private implementation of _Span? And in the future, if we decide on manual configuration of limits, a new parameter into TracerProvider?

Right. I was hesitant to create a sub-class of Tracer just to enable this. I had original added the attribute directly to Span init thinking it was not considered public API but then ended up moving it to _Span. I can add a similar _with_limits() private method to span if you think that makes more sense.

If we are officially supporting setting limits manually as a feature of OT Python, shouldn't we do things consistently by just modifying the constructors (and our api surface) and simply pass in the parameter down the chain to Span? Are we hesitant on doing this because it's not defined in the specs?

Yes, we should definitely do that but this PR isn't trying to implement that. I don't think spec is an issue here. The spec says it should be configurable via the TracerProvider but it leaves actual API to each language. Once we have a resolution for this discussion, I'll create another PR that'll remove _with_limits() and add limits as a regular attribute to the constructor.

if understood the discussion correctly, I'd prefer also having the possbility to have an individual new span to use increased limits (ie. if during design time we know and expect huge number of events etc.) but overall default limits should stay at safe numbers (to prevent bad code on other places to flood tracing)...

Hence I'd opt for

_with_limits() at creating a new span (if the scenario described above make sense to you).

having set the default limits set like
provider = TracerProvider(max_attributes_limit=100, max_events_limit=100, max_links_limit=100) (following the zen of Python where it says to keep things as easy and straight as possible from the developer perspective)

from a real scenario:
in method InvoiceExcel.validate_journal_data() I expect many data rows being validated and hence during development-time I'd like to increase Event-limits only for that single span to beyond its default of 128 but only here. For all other methods I'm fine and prefering the limit of 128 to be applied to be on the safe side and be protected should some race condition (or poor code) harm to flood the tracing.
For my code to get all validation events, I had to increase the OTEL_SPAN_EVENT_COUNT_LIMIT 'globally'. It would be nice to only increase that limit for a given span... hope this makes sense?

@owais

Once we have a resolution for this discussion, I'll create another PR that'll remove _with_limits() and add limits as a regular attribute to the constructor.

Okay so since this discussion has not been resolved yet, would it be easier to remove everything related span limit configuration through code from this PR to ge tthe "unset" feature resolved? I think some more discussion needs to be had over whether we want _with_limits.

@lzchen This PR keeps everything private and does not introduce any APIs so we can change things freely depending on the outcome of the discussion. _with_limits() was a workaround so that I didn't have to pass down the limit values to tracer constructor as that'd change the public API. I added it specifically to unblock this PR so we can discuss the public API in more detail :)

I can remove it and set limits directly on the tracer right after creating it inside tracer provider or create a private subclass of tracer and pass limits to it's constructor but I was hoping to remove _with_limits() in future and replace it with the outcome of the discussion you linked. Does that make sense? LMK if you'd prefer private sub-class or directly setting the limit attrs on tracer instance instead? happy to do either.

@SREbuilt yes, makes sense. Once we allow setting limits via tracer provider (not in this PR), you can create a dedicated tracer provider for your "special" spans and set different limits there. Any spans created with tracers fetched from that provider will respect the limits.

@lzchen removed _with_limits() for now. dddf654

@owais

Limits attr looks good for now.

- Added SpanLimits class - TracerProvider now optionally accepts an instance of SpanLimits which is passed all the way down to Spans. - Spans use the limits to created bounded lists or dicts for different fields.

owais requested review from a team, codeboten and ocelotl and removed request for a team May 11, 2021 01:43

owais force-pushed the span-limits-feature branch from 6fc4c8d to ec0044f Compare May 11, 2021 01:43

owais requested a review from lzchen May 11, 2021 01:43

owais force-pushed the span-limits-feature branch 2 times, most recently from 9db635c to c6a502f Compare May 11, 2021 01:47

owais marked this pull request as draft May 11, 2021 02:01

owais force-pushed the span-limits-feature branch 2 times, most recently from 3239307 to adca173 Compare May 11, 2021 02:04

owais mentioned this pull request May 11, 2021

Allow span limits to be unset #1830

Closed

11 tasks

owais force-pushed the span-limits-feature branch from adca173 to 660d289 Compare May 11, 2021 02:09

owais commented May 11, 2021

View reviewed changes

owais force-pushed the span-limits-feature branch 2 times, most recently from 518a6cb to 9d77737 Compare May 11, 2021 02:17

owais marked this pull request as ready for review May 11, 2021 02:44

lzchen reviewed May 11, 2021

View reviewed changes

owais requested a review from lzchen May 12, 2021 16:09

ocelotl reviewed May 13, 2021

View reviewed changes

owais force-pushed the span-limits-feature branch 4 times, most recently from 95a3891 to 3bb263f Compare May 15, 2021 00:32

owais force-pushed the span-limits-feature branch 2 times, most recently from e7e77c1 to 2f21502 Compare May 15, 2021 01:02

ocelotl suggested changes May 17, 2021

View reviewed changes

lzchen mentioned this pull request May 17, 2021

Trace SDK does not raise when Span attribute and link limits are exceeded #1856

Merged

11 tasks

owais force-pushed the span-limits-feature branch from 2f21502 to e39cfe0 Compare May 18, 2021 23:52

ocelotl suggested changes May 19, 2021

View reviewed changes

owais force-pushed the span-limits-feature branch from e39cfe0 to 4877a52 Compare May 19, 2021 00:32

owais changed the title ~~Added Span limits support to the tracing SDK~~ Allow users to "unset" SDK limits and evaluate default limits lazily instead of on import May 19, 2021

owais requested a review from ocelotl May 19, 2021 01:12

ocelotl approved these changes May 20, 2021

View reviewed changes

lzchen self-assigned this May 20, 2021

codeboten reviewed May 20, 2021

View reviewed changes

lzchen reviewed May 20, 2021

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed May 20, 2021

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed May 20, 2021

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed May 20, 2021

View reviewed changes

owais force-pushed the span-limits-feature branch 4 times, most recently from dddf654 to 1bd43a6 Compare May 24, 2021 19:31

lzchen approved these changes May 24, 2021

View reviewed changes

lzchen requested a review from ocelotl May 24, 2021 22:21

owais added 2 commits May 25, 2021 05:29

Added Span limits support to the tracing SDK

f8a445a

- Added SpanLimits class - TracerProvider now optionally accepts an instance of SpanLimits which is passed all the way down to Spans. - Spans use the limits to created bounded lists or dicts for different fields.

Remove _with_limits() from tracer

96faf0e

owais force-pushed the span-limits-feature branch from 1bd43a6 to 96faf0e Compare May 24, 2021 23:59

ocelotl approved these changes May 25, 2021

View reviewed changes

lzchen merged commit 3dbbd1b into open-telemetry:main May 25, 2021

owais deleted the span-limits-feature branch May 25, 2021 09:58

		@@ -564,6 +562,79 @@ def _format_links(links):
		return f_links


		class SpanLimits:

	- Lazily configure Span limits and allow limits limits to be configured via code.
	- Lazily configure Span limits and allow limits to be configured via code.

		def __init__(self, args, *kwargs):
		self._limits: _Limits = kwargs.pop("limits", _UnsetLimits)

Allow users to "unset" SDK limits and evaluate default limits lazily instead of on import #1839

Allow users to "unset" SDK limits and evaluate default limits lazily instead of on import #1839

Conversation

owais commented May 11, 2021 • edited Loading

Description

Please delete options that are not relevant.

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

owais commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

On the class increasing API surface.

Alternative

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais commented May 15, 2021 • edited Loading

ocelotl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais commented May 17, 2021

owais commented May 18, 2021

owais commented May 18, 2021

ocelotl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SREbuilt May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais commented May 11, 2021 •

edited

Loading

owais May 13, 2021 •

edited

Loading

owais commented May 15, 2021 •

edited

Loading

lzchen May 20, 2021 •

edited

Loading

SREbuilt May 21, 2021 •

edited

Loading

owais May 24, 2021 •

edited

Loading