Apply validation of attributes to `Resource`, move attribute related logic to util package by srikanthccv · Pull Request #1834 · open-telemetry/opentelemetry-python

srikanthccv · 2021-05-10T15:32:14Z

lzchen · 2021-05-10T21:08:29Z

What is being changed specifically that we are lacking from the specs?

srikanthccv · 2021-05-10T22:15:34Z

Resource doesn't check the types of attribute values. It either fails at the serialization or skips silently when exporting.

srikanthccv · 2021-05-10T22:23:18Z

Hmm probably title is misleading but I can't come up with better one.

lzchen · 2021-05-10T23:01:08Z

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

+
+_logger = logging.getLogger(__name__)
+
+


Should all of this logic go into the api?

Maybe if we want to provide this as part of public api package

🤔 I guess other SDKs could use a method like this to ensure their attributes are all validated and users could potentially use this to validate/filter their attributes before setting them? not sure how likely these scenarios are though. I could go either ways with this.

I was more referring to the fact that Attributes are an API concept. I don't think they need to be part of the PUBLIC api, simply private methods that exist in the api package.

Are Attributess an API concept? I am inclined towards keeping them in sdk because all operations are no-op in api package so we will probably never use them there and if we don't want offer them as a part of public api to user/application owners I don't see any benefit in moving to api package?

Yes Attribute s are defined in the API. Hmm now thinking about it, this is probably valuable and common enough to be exposed in the public api. It is also defined in the specs so I don't mind increasing our api surface for this. For example, this would be useful for instrumentations to leverage (which only have dependencies on the api).

I can move this to api util package. I just want to make sure we are okay with making these public because of all of util is pretty much private and internal.

You can be the first :)

CHANGELOG.md

Co-authored-by: Leighton Chen <lechen@microsoft.com>

codeboten · 2021-05-10T15:36:41Z

opentelemetry-sdk/tests/trace/test_trace.py

+        self.assertTrue(_is_valid_attribute_value([True, False]))
+        self.assertTrue(_is_valid_attribute_value(["ss", "dw", "fw"]))
+        self.assertTrue(_is_valid_attribute_value([]))
+        self.assertFalse(_is_valid_attribute_value(dict()))


nit: please group all the true/false together. makes it easier to read :)

codeboten · 2021-05-11T15:19:11Z

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

+
+_logger = logging.getLogger(__name__)
+
+


🤔 I guess other SDKs could use a method like this to ensure their attributes are all validated and users could potentially use this to validate/filter their attributes before setting them? not sure how likely these scenarios are though. I could go either ways with this.

owais · 2021-05-14T16:38:25Z

Instead of utils, can we create an attributes package even if all it contains are private functions. "utils" tend to grow over time and eventually defeat the purpose of having modules as nothing is discoverable anymore :)

At least a utils/attributes sub-module would be nice if we don't want to add a root level module.

lzchen · 2021-05-18T00:18:25Z

@lonewolf3739
We can talk about this in the SIG meeting.

srikanthccv · 2021-05-20T16:44:19Z

Sorry I couldn't talk during the call. I moved the related code to api and to top level attributes package instead of utils. Please review and add any comments.

codeboten

Thanks for the update 👍

lzchen · 2021-05-20T21:29:20Z

@lonewolf3739
I believe we wanted this to be in utils/attributes folder right?

owais

Can we make these functions private for now? I have a couple of in-progress PRs (#1839, #1847) that might need to use or modify these functions. Even though I plan to get them merged before next release, it'd be safer to if we don't make these public in this PR.

srikanthccv · 2021-05-21T00:18:41Z

@lonewolf3739
I believe we wanted this to be in utils/attributes folder right?

@owais suggested creating attributes package and it made more sense in terms of discoverability. I am fine with either way. What do you think?

lzchen · 2021-05-24T16:16:05Z

@lonewolf3739

@owais suggested creating attributes package and it made more sense in terms of discoverability. I am fine with either way. What do you think?

I'm okay with creating a separate package if the functions are going to be public and available to use. However, if we are making them private, I'd prefer to just have a submodule. Thoughts?

srikanthccv · 2021-05-24T16:51:52Z

We want to make them public right? That was the original discussion here #1834 (comment). Given that we only use it and probably change as need arises (Owais has some changes in other PR) do you still think we want to make them public? If we want to keep them private there is no need to have separate package.

lzchen · 2021-05-24T17:08:52Z

@lonewolf3739

Yup, I'm fine with making them public. I was just thinking about @owais ' comment, so for the interim, leaving them private for now in a module is okay with me (please make the methods private for now).

…thon into issue-1831

srikanthccv added 6 commits May 10, 2021 20:22

Update Resource attributes to comply with Attributes specification

e08e586

Fix tests

4d72201

Fix lint

b7aafd5

Fix invalid byte type

10f35c7

Bug fix

1be3c17

Remove unused import

aee5f90

srikanthccv requested review from a team, aabmass and codeboten and removed request for a team May 10, 2021 15:32

Fix lint; Add CHANGELOG entry

8b915cf

srikanthccv added 2 commits May 11, 2021 03:46

Merge branch 'main' into issue-1831

5c8b9a1

Fix lint

178d635

lzchen reviewed May 10, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

8f76e2c

Co-authored-by: Leighton Chen <lechen@microsoft.com>

srikanthccv changed the title ~~Update Resource attributes to comply with Attributes specification~~ Apply validation of attributes to Resource, move attribute related logic to util package May 10, 2021

Update CHANGELOG entry

276b339

codeboten reviewed May 11, 2021

View reviewed changes

Merge branch 'main' into issue-1831

9ab68fa

srikanthccv added 4 commits May 15, 2021 00:49

Resolve merge conflicts

0137d12

Address review comments

ba65385

Add License

5a30d10

fix lint

0a36c67

lzchen assigned owais May 20, 2021

lzchen self-assigned this May 20, 2021

codeboten approved these changes May 20, 2021

View reviewed changes

Merge branch 'main' into issue-1831

b3a4a44

owais requested changes May 20, 2021

View reviewed changes

srikanthccv added 3 commits May 24, 2021 22:55

Make methods private

b4d09db

Merge branch 'issue-1831' of github.com:lonewolf3739/opentelemetry-py…

f9b2bc1

…thon into issue-1831

Merge branch 'main' into issue-1831

026dda1

lzchen approved these changes May 24, 2021

View reviewed changes

owais approved these changes May 24, 2021

View reviewed changes

lzchen merged commit eda57f7 into open-telemetry:main May 24, 2021

srikanthccv deleted the issue-1831 branch May 24, 2021 23:15


		_logger = logging.getLogger(__name__)


		_logger = logging.getLogger(__name__)

Conversation

srikanthccv commented May 10, 2021

Uh oh!

lzchen commented May 10, 2021

Uh oh!

srikanthccv commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srikanthccv commented May 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owais commented May 14, 2021

Uh oh!

lzchen commented May 18, 2021

Uh oh!

srikanthccv commented May 20, 2021

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

lzchen commented May 20, 2021

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

srikanthccv commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzchen commented May 24, 2021

Uh oh!

srikanthccv commented May 24, 2021

Uh oh!

lzchen commented May 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

srikanthccv commented May 10, 2021 •

edited

Loading

srikanthccv commented May 21, 2021 •

edited

Loading