Adding Resource API - v0 #81

simonz130 · 2018-12-20T08:09:59Z

Added IResource definition based on
https://github.com/census-instrumentation/opencensus-proto/blob/master/src/opencensus/proto/resource/v1/resource.proto
Added abstract class that implements IResource. Currently it's a scheleton for adding custom resources.
This is needed so that the library can identify the resource for which telemetry is collected.

- Added IResource definition based on https://github.com/census-instrumentation/opencensus-proto/blob/master/src/opencensus/proto/resource/v1/resource.proto - Added abstract class that implements IResource. Currently it's a scheleton for adding custom resources. This is needed so that the library can identify the resource for which telemetry is collected.

src/OpenCensus/Resource/Resource.cs

SergeyKanzhelev · 2018-12-20T17:01:50Z

src/OpenCensus/Resource/Resource.cs

+                    }
+
+                    string key = keyValuePair[0].Trim();
+                    string value = Regex.Replace(keyValuePair[1].Trim(), "^\"|\"$", string.Empty);


two calls to replace will be more efficient. At least compile the regex so it is not created every cycle of the loop

static Replace method doesn't work with Regex instances (pre-compiled), so I don't see a point in regex pre-compilation here. Also, I don't create a Regex instance every loop iteration - I pass a simple regex with two patterns to match and get a string back.

src/OpenCensus/Utils/Arguments.cs

SergeyKanzhelev · 2018-12-20T18:37:42Z

src/OpenCensus/Utils/Arguments.cs

+        /// <returns>Whether condition is true</returns>
+        public static bool Check(bool condition, string errorMessage)
+        {
+            if (!condition)


this is one-liner in code. Do you need a separate method defined?

We need some form of arguments checking and then either throwing exception or logging in invalid cases. We need this logic throughout the public API surface. So I prefer to have it in one place even if it's one-liner right now than to spread it in multiple places. Besides, there will be additional overloads soon, so I laid the foundation now.

What other overloads are you anticipating? From my expirience these helper methods only complicates understanding the code. You never know will it throw or ignore an error for instance.

As far as I understand, this is a simplified guava preconditions implementation is C#. It would be cool to have something like that on the platform or common-lib level, but unfortunately, there is nothing like that and couple 3rd party I've found - CuttingEdge.Conditions and Preconditions.NET are not really popular.

We also don't get any static code analysis from this when we implement them on our own.

The problem here is not only readability but also perf. A caller has to create an error string (which is not const) and sometimes do a bunch of operations to make error message descriptive enough (add values of Ids, some context).
And this string will be constructed even when the condition is false (almost always). So such checks should not be used on the hot path anyways.

So I tend to agree with Sergey, there are certain issues with this approach and I'd prefer we explicitly throw or write log in the code where it's needed.

OK, I'm removing this in the meantime.

src/OpenCensus/Resource/Resource.cs

SergeyKanzhelev · 2018-12-26T21:41:40Z

src/OpenCensus.Abstractions/Resource/IResource.cs

+    /// of resource information from the environment and progressive population as signals propagate from
+    /// the core instrumentation library to a backend's exporter.
+    /// </summary>
+    public interface IResource


is this interface supposed to be read only? If so - change IList to IEnumerable please.

It is supposed to be readonly, but IEnumerable doesn't have Count and the types that will implement the interface can differ wildly (Azure/AWS/GCP resources). Also, typically, the tags will be stored in a list anyway.

Why do you need Count? There is Count() method in Linq namespace if needed. But the common use case will be to enumerate, not index these, correct?

See also PR related to this issue: #64

SergeyKanzhelev · 2018-12-26T21:43:17Z

src/OpenCensus/Resource/Resource.cs

+        /// </summary>
+        /// <param name="rawEnvironmentTags">Environment tags as a raw, comma separated string</param>
+        /// <returns>Environment Tags as a list</returns>
+        private static ITag[] ParseResourceLabels(string rawEnvironmentTags)


Can you please add some unit tests as part of this PR?

src/OpenCensus.Abstractions/Resource/IResource.cs

SergeyKanzhelev · 2018-12-26T21:45:55Z

src/OpenCensus/Utils/Arguments.cs

+        /// <returns>Whether condition is true</returns>
+        public static bool Check(bool condition, string errorMessage)
+        {
+            if (!condition)


What other overloads are you anticipating? From my expirience these helper methods only complicates understanding the code. You never know will it throw or ignore an error for instance.

src/OpenCensus/Resource/Resource.cs

lmolkova · 2018-12-26T22:50:41Z

src/OpenCensus/Resource/Resource.cs

+                    labels.Add(new Tag(new TagKey(key), new TagValue(value)));
+                }
+
+                return labels.ToArray();


this is an extra allocation, could we avoid it by using the same type everywhere? (return type, private field, IResource.Tags)

If you mean TagKey and TagValue constructors - I've fixed that. If you mean labels.ToArray call - there is not much you can do. The number of labels is not known in advance so you have to parse them in runtime. I could return a list but once labels were calculated, they should stay immutable. Finally, this calculation happens once in a lifetime of the process.

lmolkova

Left some minor comments. The biggest concern is IList<ITag> Tags: what operations are expected on the Tags and why it's not IEnumerable

* snupkg, update the linux image and remove travis * removed publish command. It's unnecessary * remove duplicated commands * added comment * use buildProperties instead of arguments * removed comment and updated to ANY 2.2 * latest 2.2

- Added IResource definition based on https://github.com/census-instrumentation/opencensus-proto/blob/master/src/opencensus/proto/resource/v1/resource.proto - Added abstract class that implements IResource. Currently it's a scheleton for adding custom resources. This is needed so that the library can identify the resource for which telemetry is collected.

* readme and changelog updates * fix issues

* fix the license and apply NuGet/Announcements#32 * missed a few files

…re (census-instrumentation#80) * better name * make it work * revert wrong addition * addressed code review * fix build failure and added comment for the sampler * added changelog entry * added route into the attributs as well

* few IList replaced with IEnumerable * parent links from IList to IEnumerable * a few more of IList -> IEnumerable * one more * stats event with IEnumerable now * smal utils method should not use IList * another utility method

… a dependency on it (census-instrumentation#86)

…ion#85) * documents * first warning was implemented * first version of docs complete

…ecking 2) Added a few events logging 3) Added tests for Resource parsing (labels and resource type)

…into AddResource

googlebot · 2019-01-03T09:37:33Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

…into AddResource

src/OpenCensus/Implementation/OpenCensusEventSource.cs

SergeyKanzhelev

It is almost ready for merge. Would you please update error messages. Also it feels for me that IEnumerable is still preferable type there.

src/OpenCensus/Implementation/OpenCensusEventSource.cs

simonz130 · 2019-01-03T22:29:52Z

I've changed from IList to IEnumerable, but I still think that it's better to use IList when the meaning is "holding a list of elements" to allow not only iterating using foreach loops or Linq but also regular for loops that are more performant.

- Changed IList to IEnumerable - Refined error messages to more details.

simonz130 · 2019-01-03T23:32:42Z

Can you please merge it?

googlebot · 2019-01-03T23:36:18Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

I think all feedback was addressed

SergeyKanzhelev suggested changes Dec 20, 2018

View reviewed changes

Addressing comments - mainly avoiding exceptions

b885aee

SergeyKanzhelev suggested changes Dec 26, 2018

View reviewed changes

lmolkova reviewed Dec 26, 2018

View reviewed changes

src/OpenCensus/Resource/Resource.cs Outdated Show resolved Hide resolved

lmolkova reviewed Dec 26, 2018

View reviewed changes

src/OpenCensus/Resource/Resource.cs Outdated Show resolved Hide resolved

lmolkova reviewed Dec 26, 2018

View reviewed changes

lmolkova previously requested changes Dec 26, 2018

View reviewed changes

SergeyKanzhelev and others added 11 commits January 2, 2019 22:31

Addressing comments - mainly avoiding exceptions

3f19aff

Catch up docs updates after release (census-instrumentation#79)

540fea6

* readme and changelog updates * fix issues

fix the license warnings (census-instrumentation#83)

cfeb601

* fix the license and apply NuGet/Announcements#32 * missed a few files

allow net45 compilation for abstractions so some common SDKs may take…

594a032

… a dependency on it (census-instrumentation#86)

Event logging - docs and a single event example. (census-instrumentat…

4c19c0b

…ion#85) * documents * first warning was implemented * first version of docs complete

1) Removed argument.check method and replaced with direct argument ch…

76c20af

…ecking 2) Added a few events logging 3) Added tests for Resource parsing (labels and resource type)

Merge branch 'AddResource' of github.com:simonz130/opencensus-csharp …

2d1d55e

…into AddResource

Simon Zeltser added 3 commits January 3, 2019 01:38

Merge branch 'develop' into AddResource

a0ea9e0

Fixing build break

306469a

Merge branch 'AddResource' of github.com:simonz130/opencensus-csharp …

5662582

…into AddResource

SergeyKanzhelev reviewed Jan 3, 2019

View reviewed changes

src/OpenCensus/Implementation/OpenCensusEventSource.cs Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed Jan 3, 2019

View reviewed changes

src/OpenCensus/Implementation/OpenCensusEventSource.cs Outdated Show resolved Hide resolved

src/OpenCensus/Implementation/OpenCensusEventSource.cs Outdated Show resolved Hide resolved

src/OpenCensus/Implementation/OpenCensusEventSource.cs Outdated Show resolved Hide resolved

Simon Zeltser added 2 commits January 3, 2019 14:32

Addressed comments:

59d0787

- Changed IList to IEnumerable - Refined error messages to more details.

Merge branch 'develop' into AddResource

c3c61b8

SergeyKanzhelev approved these changes Jan 3, 2019

View reviewed changes

songy23 added cla:yes cla: yes and removed cla:yes labels Jan 3, 2019

SergeyKanzhelev merged commit 68bd833 into census-instrumentation:develop Jan 4, 2019

simonz130 deleted the AddResource branch January 4, 2019 06:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Resource API - v0 #81

Adding Resource API - v0 #81

simonz130 commented Dec 20, 2018

SergeyKanzhelev Dec 20, 2018

simonz130 Dec 20, 2018

SergeyKanzhelev Dec 20, 2018

simonz130 Dec 20, 2018

SergeyKanzhelev Dec 26, 2018

lmolkova Dec 26, 2018

simonz130 Jan 3, 2019

SergeyKanzhelev Dec 26, 2018

simonz130 Jan 3, 2019

SergeyKanzhelev Jan 3, 2019

SergeyKanzhelev Jan 3, 2019

SergeyKanzhelev Dec 26, 2018

simonz130 Jan 3, 2019

SergeyKanzhelev Dec 26, 2018

lmolkova Dec 26, 2018 •

edited

Loading

simonz130 Jan 3, 2019

lmolkova left a comment

googlebot commented Jan 3, 2019

SergeyKanzhelev left a comment

simonz130 commented Jan 3, 2019

simonz130 commented Jan 3, 2019

googlebot commented Jan 3, 2019

Adding Resource API - v0 #81

Adding Resource API - v0 #81

Conversation

simonz130 commented Dec 20, 2018

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

lmolkova Dec 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova left a comment

Choose a reason for hiding this comment

googlebot commented Jan 3, 2019

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

simonz130 commented Jan 3, 2019

simonz130 commented Jan 3, 2019

googlebot commented Jan 3, 2019

lmolkova Dec 26, 2018 •

edited

Loading