Resource sdk Implementation #502

lalitb · 2021-01-05T16:30:25Z

Initial release of Resource SDK .

Supported -

Create resource
Merge resource
Empty resource creation
Base resource detection package.
Specify resource information from env variable ( OTEL_RESOURCE_ATTRIBUTES )
Integrating with TracerProvider and Traces

To be added as separate PR:

Adding in SpanData and propagating to exporters.
Resource detection logic from multiple resources, and merging all.

#334

codecov · 2021-01-05T16:41:35Z

Codecov Report

Merging #502 (8cd247e) into master (ad9e57f) will decrease coverage by 0.02%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
- Coverage   94.45%   94.42%   -0.03%     
==========================================
  Files         189      194       +5     
  Lines        8388     8509     +121     
==========================================
+ Hits         7923     8035     +112     
- Misses        465      474       +9

Impacted Files	Coverage Δ
sdk/include/opentelemetry/sdk/trace/tracer.h	`100.00% <ø> (ø)`
.../include/opentelemetry/sdk/trace/tracer_provider.h	`100.00% <ø> (ø)`
sdk/src/trace/span.h	`100.00% <ø> (ø)`
sdk/src/trace/tracer_provider.cc	`65.00% <50.00%> (-5.59%)`	⬇️
sdk/src/resource/resource.cc	`88.00% <88.00%> (ø)`
sdk/test/resource/resource_test.cc	`95.65% <95.65%> (ø)`
ext/test/zpages/tracez_data_aggregator_test.cc	`97.35% <100.00%> (+<0.01%)`	⬆️
ext/test/zpages/tracez_processor_test.cc	`98.71% <100.00%> (+<0.01%)`	⬆️
sdk/include/opentelemetry/sdk/resource/resource.h	`100.00% <100.00%> (ø)`
...ude/opentelemetry/sdk/resource/resource_detector.h	`100.00% <100.00%> (ø)`
... and 12 more

pyohannes · 2021-01-07T01:55:41Z

sdk/include/opentelemetry/sdk/resource/resource.h

+
+  std::unique_ptr<Resource> Merge(const Resource &other);
+
+  static std::unique_ptr<Resource> Create(


Why do we have both Create methods and constructors?

I see, probably because of creating unique pointers. Should we make the constructors private then, to prevent the creation of unmanaged resources?

There is challenge to do this in C++. Ideally, I would like to make constructor as private, and make OTELResourceDetector as friend class of Resource, but we actually need all derivations of ResourceDetector have access to this constructor.
opentelementry-java does it using AutoValue annotation, and opentelemetry-dotnet does it using internal keyword.

Ok. Please ignore above comment. I have now made constructor as protected ( so that it can be used in test cases ), and made OTELResourceDetector as friend class. It seems we don't need to invoke Resource constructor in any other detector, so we should be fine.

pyohannes · 2021-01-07T02:07:26Z

sdk/include/opentelemetry/sdk/resource/resource.h

+  Resource(const opentelemetry::common::KeyValueIterable &attributes) noexcept;
+
+  Resource(const opentelemetry::sdk::trace::AttributeMap &attributes) noexcept;
+
+  template <class T,
+            nostd::enable_if_t<common::detail::is_key_value_iterable<T>::value> * = nullptr>
+  Resource(const T &attributes) noexcept : Resource(common::KeyValueIterableView<T>(attributes))
+  {}
+
+  Resource(const std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>
+               attributes) noexcept
+      : Resource(nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>{
+            attributes.begin(), attributes.end()})
+  {}


What's your opinion about replacing those with a single:

Resource(const std::map<std::string, opentelemetry::sdk::trace::SpanDataAttributeValue> attributes) noexcept;

As resources are an SDK only concept, we don't need to worry about ABI compatibility here.

Also, we should move the attribute_utils.h (containing the AttributeMap) from trace to common. Resources will be used by metrics as well.

Good point,
Using std::unordered_map<std::string, opentelemetry::sdk::common::OwnedAttributeValue directly, and getting rid of AttributeMap.

pyohannes · 2021-01-07T02:31:37Z

sdk/src/resource/resource.cc

+  opentelemetry::sdk::trace::AttributeMap resource_attributes;
+  std::istringstream iss(attributes);
+  std::string token;
+  while (std::getline(iss, token, ','))
+  {
+    size_t pos        = token.find('=');
+    std::string key   = token.substr(0, pos);
+    std::string value = token.substr(pos + 1);
+    resource_attributes.SetAttribute(key, value);
+  }
+  return std::unique_ptr<Resource>(new Resource(resource_attributes));


I think it would be better to have this logic in the OTELResourceDetector, as it's related to the format of the environment variable.

Correct. Have changed this accordingly now.

pyohannes · 2021-01-07T02:32:59Z

sdk/src/resource/resource.cc

+  return default_resource.Merge(*(OTELResourceDetector().Detect()));
+}
+
+std::unique_ptr<Resource> Resource::CreateEmpty()


I think when we start to use resources, we have to make them shared pointers. Resources are supposed to be attached to every span. We don't want to have unique pointers there, but share a common resource with a shared pointer.

Right. Have changed std::unique_ptr to std::shared_ptr_ptr now.

Ok. based on suggestion from @jsuereth , Resources::Merge() and Resources::Create() are modified to return stack allocated Resource by value. And this resource is then owned by sdk::tracer_provider and passed as reference to span. Please refer to examples/simple/main.cc in this PR on how the usage would be, and let me know if any concerns.

pyohannes · 2021-01-07T02:33:44Z

sdk/src/resource/resource.cc

+
+std::unique_ptr<Resource> Resource::CreateEmpty()
+{
+  return Create({});


Maybe we can use a static singleton for the default resource, like Java does: https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/common/src/main/java/io/opentelemetry/sdk/resources/Resource.java#L53

Good point. Using static local for GetDefault() and GetEmpty() now.

jsuereth · 2021-01-05T23:20:04Z

sdk/include/opentelemetry/sdk/resource/resource_detector.h

+class ResourceDetector
+{
+public:
+  virtual std::unique_ptr<opentelemetry::sdk::resource::Resource> Detect() = 0;


Is there a reason to return a unique_ptr over just using the stack here?

Resource is non-virtual class, so it might be a simple API to just return it by-value.

I think the motivation behind that is to avoid excessive copying of a resource. A resource might be appended to each span. Having a shared_ptr (I think a unique pointer will not do) enables us to avoid copying the resource and its attributes.

Based on suggestion from @jsuereth , Resources::Merge() and Resources::Create() are modified to return stack allocated Resource by value. And this resource is then owned by sdk::tracer_provider and passed as reference to span. Please refer to examples/simple/main.cc in this PR on how the usage would be, and let me know if any concerns.

jsuereth · 2021-01-05T23:27:23Z

sdk/include/opentelemetry/sdk/resource/resource.h

+
+  const opentelemetry::sdk::trace::AttributeMap &GetAttributes() const noexcept;
+
+  std::unique_ptr<Resource> Merge(const Resource &other);


NIt: Here and elsewhere in this file. Can we add method documentation about the expectations of calling this?

E.g. for Merge I expect attributes already defined to not be overridden by other's attributes, but want to confirm this is expected.

For this review, yes I'm looking it up in the spec, but we should document it here for other maintainers of C++.

Sure. Will add the method documentation.

I have added the documentation now.

jsuereth · 2021-01-11T14:37:34Z

sdk/include/opentelemetry/sdk/resource/resource.h

+
+  const ResourceAttributes &GetAttributes() const noexcept;
+
+  std::shared_ptr<Resource> Merge(const Resource &other);


Why not just return a stack-allocated resource here? Wouldn't that be a simpler api?

Similar for Create.

As it is possible to share resources across multiple spans, it would be better to use shared_ptr and avoid un-necessary copying.

This method doesn't need to determine how resources are shared.

A few points:

Resource-attachment to Trace is NOT specified in the API. In fact, I don't think you can attach via the API in any other language.

The resource will be attached to a metric provider or trace provider (see: https://github.com/open-telemetry/opentelemetry-java/blob/efd559f1e09e02f6f5a93ac91c272042849b3bd1/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OpenTelemetrySdkAutoConfiguration.java#L35) for an example in Java.

I think it's reasonable for this method to return on the Stack. We then have our MetricProvider + TraceProvider have a Resource attribute they own, and Trace/Metrics generated from those provides can use Pointers or references to the resource associated with a provider.

Resources should really only be a startup concern and not something we expect users to pass around. I'd rather see a Resource&& argument on *Provider classes just to make ownership clear. This is similar to other APIs.

References:

Trace API spec has no reference to Resource

Trace SDK spec denotes the resource is implicitly associated with a Span.

I agree here. Resource need to be associated with TracerProvider at sdk level, and we can actually pass the reference of same to traces generated from the provider. And in that case, we don't need to manage smart pointers. Let me add other half in this PR, and it would be easier to comprehend.

OK, Resources::Merge() and Resources::Create() are modified to return stack allocated Resource by value. And this resource is then owned by sdk::tracer_provider and passed as reference to span. Please refer to examples/simple/main.cc in this PR on how the usage would be, and let me know if any issues now.

jsuereth · 2021-01-11T14:38:15Z

sdk/include/opentelemetry/sdk/resource/resource_detector.h

+class ResourceDetector
+{
+public:
+  virtual std::shared_ptr<opentelemetry::sdk::resource::Resource> Detect() = 0;


Same nit: Is there a reason to return a shared_ptr here? AFAIK Resource detection happens once and generally isn't passing pointers between different memory owners.

As mentioned in earlier comment - As it is possible to share resources across multiple spans, it would be better to use shared_ptr and avoid un-necessary copying.

I'd like to avoid the counting of shared_ptr references if we can just use the attached TraceProvider.

I.e. shared_ptr has a cost, and I think we don't need it in this scenario. If we still want to use shared_ptr within the SDK to associate resource from *Provider to Span, that's fine but a different PR.

I think without seeing the other half of the change it's hard to fully comment on merits of shared_ptr.

Yes, I did revisit again, and probably we mayn't need smart pointer here ( including unique_ptr). Let me add other half of implementation in this PR, so that it would be easy to comprehend and review.

Changes are done as advised ( refer above comments ).

jsuereth · 2021-01-11T14:43:59Z

sdk/src/resource/resource.cc

+  {
+    Resource tmp_resource(attributes);
+    auto merged_resource = tmp_resource.Merge(default_resource);
+    return merged_resource->Merge(*(OTELResourceDetector().Detect()));


Nit:

Would it make sense to have both the OTEL resource + default resource be statics within this function?

{ static auto default_resource = Resource::GetDefault(); static auto otel_env_resource = OTELResourceDetector().Detect(); ... } This would avoid re-running the OTEL resource detection for every created resource.

Good point, I have changed it accordingly.

static auto otel_resource = OTELResourceDetector().Detect(); auto default_resource = Resource::GetDefault();

and

Resource &Resource::GetDefault() { static Resource default_resource({{TELEMETRY_SDK_LANGUAGE, "cpp"}, {TELEMETRY_SDK_NAME, "opentelemetry"}, {TELEMETRY_SDK_VERSION, OPENTELEMETRY_SDK_VERSION}}); return default_resource; }

jsuereth · 2021-01-11T14:45:07Z

sdk/include/opentelemetry/sdk/resource/resource.h

+class Resource
+{
+public:
+  Resource(const ResourceAttributes &attributes = ResourceAttributes()) noexcept;


The specification says that ANY created resource should go through the environment variable additions + default logic you have in Create. Should we hide this constructor to only the OTELResourceDetector + getDefault ?

There is challenge to do this in C++. Ideally, I would like to make constructor as private, and make OTELResourceDetector as friend class of Resource, but we actually need all derivations of ResourceDetector have access to this constructor.
opentelementry-java does it using AutoValue annotation, and opentelemetry-dotnet does it using internal keyword.

Ok. Please ignore above comment. I have now made constructor protected ( so that it can be used in test cases ), and made OTELResourceDetector as its friend class. It seems we don't need to invoke Resource constructor in any other detector, so we should be fine.

ThomsonTan · 2021-01-13T07:26:30Z

sdk/include/opentelemetry/sdk/resource/resource.h

+#include "opentelemetry/sdk/version/version.h"
+#include "opentelemetry/version.h"
+
+#include <cstdlib>


Is this include necessary?

No, we actually need it in resource.cpp ( to read env variable through std::getenv ). Will move it there. Thanks for noticing it.

This is done now.

maxgolov · 2021-01-14T20:37:51Z

sdk/include/opentelemetry/sdk/resource/resource.h

+   * @returns the newly merged Resource.
+   */
+
+  std::shared_ptr<Resource> Merge(const Resource &other) noexcept;


Do we have to implement these (internal to SDK surface) methods.. can we make it a map - inherited from standard library map (either map or unordered_map), and follow semantics of existing APIs available, e.g. map::merge .. Then we are not inventing some new Visual Basic-style API, but follow existing Standard Library semantics everywhere. This will be easier to comprehend by experienced modern C++ developer... Plus - I think it would be consistent if our container classes are compatible with existing algorithm templates described here

The spec defines some strict rules around merging, which I think are not satisfied by map::merge:

If the value on the primary resource is an empty string, the result has the value of the secondary resource.

Otherwise, the value of the primary resource is used.

maxgolov · 2021-01-14T20:38:57Z

sdk/test/resource/resource_test.cc

+TEST(ResourceTest, create)
+{
+
+  std::map<std::string, std::string> expected_attributes = {


Can you supplement this with a test that handles strongly-typed attributes of numeric (integer) type? i.e. map of variants. Also - the cost parameter here should be a double, not string. Somebody who is onboarding to SDK should be able to freely express strongly-typed parameters in general. Just like it's done in nlohmann/json, where you'd provide developers with initializer list constructor, that is capable of accepting arbitrary variant types described within the OpenTelemetry spec.. Maybe not going as far as implementing the array types. But at least all primitive types in spec + std::string.

Yes, sure that would be good test to showcase. I will add it.

Have modified the test to include both integer and double resource attributes. Please see if this is fine.

… resources

jsuereth

This looks really good now! Just a few minor nits.

jsuereth · 2021-01-20T18:42:49Z

sdk/include/opentelemetry/sdk/trace/tracer.h

-                  std::shared_ptr<Sampler> sampler = std::make_shared<AlwaysOnSampler>()) noexcept;
+                  std::shared_ptr<Sampler> sampler = std::make_shared<AlwaysOnSampler>(),
+                  const opentelemetry::sdk::resource::Resource &resource =
+                      opentelemetry::sdk::resource::Resource::Create({})) noexcept;


I think this one may leak a nullptr exception from the default value.

I.e. the default argument won't be retained. AFAIK it should be ok to not give this argument a default value and require the TracerProvider to pass one into the tracer.

Right, default argument for reference can leak. Have removed default now.

jsuereth · 2021-01-20T18:47:28Z

sdk/src/trace/span.cc

@@ -108,6 +109,7 @@ Span::Span(std::shared_ptr<Tracer> &&tracer,
  recordable_->SetSpanKind(options.kind);
  recordable_->SetStartTime(NowOr(options.start_system_time));
  start_steady_time = NowOr(options.start_steady_time);
+  // recordable_->SetResource(resource_); TODO


As an alternative idea (that might be harder?) we could instead push the Resource into the Exporter via the Processor. I think that's not really specified (and actually the lack of specification around HOW Resource propagates may give us room to do something different if you think it'd be cleaner).

This approach looks fine, just wanted to mention that you have options AFAICT from the spec.

Another possibility: one could just iterate over the attributes in the resource and add them to the recordable at this point. I think that's the least amount of work that would make this PR functionally complete, and we'd have the ability to elaborate on that later on.

I like the idea about propagating the resource into the exporter (that would make it possible to do some optimization, some vendors have a section of common attributes for batches), on the other hand it would make processor and exporter interfaces and the lives of exporter developers harder.

@jsuereth - Good point, this way we don't need to propagate resource every time as part of span, but there would be extra task at exporter side to integrate this resource with span data before exporting.
@pyohannes - My initial plan was to add Recordable::SetResource(). But let me explore more and add propagation in next PR as planned if that's fine ?

pyohannes · 2021-01-21T02:51:20Z

examples/batch/CMakeLists.txt

@@ -4,4 +4,5 @@ add_executable(batch_span_processor_example main.cc)

 target_link_libraries(
  batch_span_processor_example ${CMAKE_THREAD_LIBS_INIT} ${CORE_RUNTIME_LIBS}
-  opentelemetry_exporter_ostream_span opentelemetry_trace)
+  opentelemetry_exporter_ostream_span opentelemetry_trace
+  opentelemetry_resources)


Can we link this into opentelemetry_trace, like we do opentelemetry_common? Then we wouldn't need to specify it alongside opentelemetry_trace everywhere. opentelemetry_trace has a hard dependency on opentelemetry_resource anyway.

Good point, done.

pyohannes · 2021-01-21T02:58:04Z

sdk/include/opentelemetry/sdk/trace/tracer.h

@@ -57,6 +60,7 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th
 private:
  opentelemetry::sdk::AtomicSharedPtr<SpanProcessor> processor_;
  const std::shared_ptr<Sampler> sampler_;
+  const opentelemetry::sdk::resource::Resource &resource_;


I think we shouldn't make this a reference, but hold the resource by value. Theoretically, we support using a Tracer without a TracerProvider (I remember that was a requirement that came up early in the design process). Having a copy of the resource in the Tracer makes this design safer and doesn't cost much.

pyohannes · 2021-01-21T03:04:25Z

sdk/src/resource/resource.cc

+  ResourceAttributes merged_resource_attributes(attributes_);
+  for (auto &elem : other.attributes_)
+  {
+    if ((merged_resource_attributes.find(elem.first) == merged_resource_attributes.end()) ||
+        (nostd::holds_alternative<std::string>(attributes_[elem.first]) &&
+         nostd::get<std::string>(attributes_[elem.first]).size() == 0))
+    {
+      merged_resource_attributes[elem.first] = elem.second;
+    }
+  }
+  return Resource(merged_resource_attributes);


There were some recent spec changes that require this to be revisited: open-telemetry/opentelemetry-specification#1345

Thanks for noticing it. It's kind of reverse of original logic. Have incorporated these changes.

pyohannes · 2021-01-21T03:13:48Z

sdk/src/trace/span.cc

@@ -108,6 +109,7 @@ Span::Span(std::shared_ptr<Tracer> &&tracer,
  recordable_->SetSpanKind(options.kind);
  recordable_->SetStartTime(NowOr(options.start_system_time));
  start_steady_time = NowOr(options.start_steady_time);
+  // recordable_->SetResource(resource_); TODO


Another possibility: one could just iterate over the attributes in the resource and add them to the recordable at this point. I think that's the least amount of work that would make this PR functionally complete, and we'd have the ability to elaborate on that later on.

I like the idea about propagating the resource into the exporter (that would make it possible to do some optimization, some vendors have a section of common attributes for batches), on the other hand it would make processor and exporter interfaces and the lives of exporter developers harder.

ThomsonTan · 2021-01-21T17:23:04Z

sdk/include/opentelemetry/sdk/resource/resource.h

+
+  static Resource &GetDefault();
+
+protected:


Is protected necessary or just private is fine?

protected helps creating unit test case - to subclass Resource and create instance of it for testing. We are doing this in resources_test.cc. I have modified the comment to indicate it's protected :)

ThomsonTan · 2021-01-21T17:38:08Z

sdk/src/resource/resource.cc

+namespace resource
+{
+
+const std::string TELEMETRY_SDK_LANGUAGE = "telemetry.sdk.language";


Name the constants like kTelemetrySdkLanguage?

Thanks. fixed now :)

…resources

pyohannes

Looks good to me.

sdk/src/resource/resource.cc

Co-authored-by: Johannes Tax <johannes@johannes.tax>

lalitb added 6 commits January 5, 2021 17:15

resource api

a27a5e4

format and add tests

abe8287

fix bazel

083b169

fix bazel another try

b61acdd

more fixes

4540bc5

fix resouce interface constructor

dfe857c

lalitb requested a review from a team January 5, 2021 16:30

Merge branch 'master' into resources

0ae679e

pyohannes reviewed Jan 7, 2021

View reviewed changes

fix review comments

7673007

jsuereth requested changes Jan 11, 2021

View reviewed changes

lalitb added 11 commits January 12, 2021 00:14

revert change

d5b389f

revert change

9fbe8be

revert change

208f231

fix merge conflict

60034d9

Merge branch 'master' into resources

04e9de8

fix attributes_utils.h

bfdd7e1

fix review comments

2abd049

fix attributes

f71a47c

add doc

f7e4909

fix static

bc3bebc

fix test

48a30d4

ThomsonTan reviewed Jan 13, 2021

View reviewed changes

maxgolov reviewed Jan 14, 2021

View reviewed changes

lalitb added 4 commits January 17, 2021 22:11

review comments

d8d015a

fix bazel build

48ab9ea

remove un-necesaary include

f515165

add test for different resoure types

5b01258

lalitb added 6 commits January 18, 2021 11:28

fix otlp

e9745f1

Merge branch 'master' into resources

84d5405

fix valgrind error

01fd510

wMerge branch 'resources' of github.com:lalitb/opentelemetry-cpp into…

c60fefc

… resources

modify simple trace example to use resource

b023c2a

fix simple example

1586018

jsuereth approved these changes Jan 20, 2021

View reviewed changes

pyohannes reviewed Jan 21, 2021

View reviewed changes

ThomsonTan reviewed Jan 21, 2021

View reviewed changes

lalitb added 6 commits January 21, 2021 23:25

make resource ctor private

9158d71

Merge branch 'master' into resources

db2412b

fix access

35dbf99

Merge branch 'resources' of github.com:lalitb/opentelemetry-cpp into …

5e67443

…resources

fixes

adbd86d

fix comment

ef45994

pyohannes approved these changes Jan 22, 2021

View reviewed changes

sdk/src/resource/resource.cc Outdated Show resolved Hide resolved

lalitb and others added 2 commits January 22, 2021 09:53

Update sdk/src/resource/resource.cc

a272b92

Co-authored-by: Johannes Tax <johannes@johannes.tax>

Merge branch 'master' into resources

8cd247e

lalitb merged commit ce89aa1 into open-telemetry:master Jan 22, 2021

lalitb mentioned this pull request Feb 9, 2021

Add Resources to Recordable interface. #570

Closed

jsuereth mentioned this pull request Feb 26, 2021

Create SDK TRACE context, a shared reference to tracer pipelines between Tracer/TracerProvider. Update tests. #590

Closed


		std::unique_ptr<Resource> Merge(const Resource &other);

		static std::unique_ptr<Resource> Create(


		const opentelemetry::sdk::trace::AttributeMap &GetAttributes() const noexcept;

		std::unique_ptr<Resource> Merge(const Resource &other);


		const ResourceAttributes &GetAttributes() const noexcept;

		std::shared_ptr<Resource> Merge(const Resource &other);

Resource sdk Implementation #502

Resource sdk Implementation #502

Conversation

lalitb commented Jan 5, 2021 • edited Loading

codecov bot commented Jan 5, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Jan 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Jan 18, 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

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

lalitb Jan 15, 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

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

lalitb Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

lalitb Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

ThomsonTan Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

lalitb Jan 13, 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

maxgolov Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuereth 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Jan 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

pyohannes left a comment

Choose a reason for hiding this comment

lalitb commented Jan 5, 2021 •

edited

Loading

codecov bot commented Jan 5, 2021 •

edited

Loading

lalitb Jan 12, 2021 •

edited

Loading

lalitb Jan 18, 2021 •

edited

Loading

lalitb Jan 15, 2021 •

edited

Loading

lalitb Jan 11, 2021 •

edited

Loading

lalitb Jan 12, 2021 •

edited

Loading

ThomsonTan Jan 13, 2021 •

edited

Loading

lalitb Jan 13, 2021 •

edited

Loading

maxgolov Jan 14, 2021 •

edited

Loading

lalitb Jan 21, 2021 •

edited

Loading