Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Remove blacklist sensitive language#977

Merged
lzchen merged 5 commits intocensus-instrumentation:masterfrom
lzchen:exclude
Dec 9, 2020
Merged

Remove blacklist sensitive language#977
lzchen merged 5 commits intocensus-instrumentation:masterfrom
lzchen:exclude

Conversation

@lzchen
Copy link
Copy Markdown
Contributor

@lzchen lzchen commented Dec 4, 2020

We will be using "Exclude list" instead to refer to endpoints that are not going to be traced.

@lzchen lzchen requested review from a team, aabmass, c24t, hectorhdzg, reyang and songy23 as code owners December 4, 2020 15:00
@google-cla google-cla bot added the cla: yes label Dec 4, 2020
@lzchen lzchen closed this Dec 4, 2020
@lzchen lzchen reopened this Dec 4, 2020

BLACKLIST_PATHS = 'BLACKLIST_PATHS'
BLACKLIST_HOSTNAMES = 'BLACKLIST_HOSTNAMES'
EXCLUDELIST_PATHS = 'EXCLUDELIST_PATHS'
Copy link
Copy Markdown

@nilebox nilebox Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this is a breaking change?
@aabmass please make sure that this is okay / we bump major release version and mention this in the changelog if we decide to merge this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The packages that are breaking will be flask, django and pyramid. Giving the current versioning strategy (we update the packages that change with the core Opencensus package), which is currently at v0.7.10, what should we update this to?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we bump the major version to v1.7.0, it wouldn't really make sense in the future if other packages change and we match them to a new major version. Should we not have packages be updated in lockstep?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For semver major versions < 1, minor version bumps are not compatible: https://semver.org/#spec-item-4. So from semver perspective, we don't need to do a major version bump.

If users are using the "compatible release" specifier (or stricter) like opencensus-foo ~= 0.7.0, it will not automatically start installing the next minor version.

Copy link
Copy Markdown
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nilebox I agree this isn't great for users, but I think from a versioning point of view it's ok. I remember there being some OpenCensus breaking changes guidelines where you should give 30 day deprecation notice -- do you remember if those were project-wide or just for one specific repo?

@nilebox
Copy link
Copy Markdown

nilebox commented Dec 8, 2020

I remember there being some OpenCensus breaking changes guidelines where you should give 30 day deprecation notice -- do you remember if those were project-wide or just for one specific repo?

I think it was documented in some repo, but couldn't find it.

@lzchen lzchen merged commit 2d16ea4 into census-instrumentation:master Dec 9, 2020
@lzchen lzchen deleted the exclude branch December 9, 2020 00:10
This was referenced Jan 14, 2021
@tpyo
Copy link
Copy Markdown
Contributor

tpyo commented Jan 18, 2021

Think the breaking change could've been communicated in the changelog.

@lzchen
Copy link
Copy Markdown
Contributor Author

lzchen commented Jan 18, 2021

@tpyo
Thanks for catching this. Updated the release notes.

@minotru
Copy link
Copy Markdown

minotru commented Jan 26, 2021

Hello,
although semver has a special note about < 1 version, I have always (and many developers too, I guess) thought that x.y.z+1 version indicates bug fixes or internal changes without public API changes, while this parameter name change introduced breaking change. So using x.y+1 version might be a better idea, I guess.

I agree this isn't great for users, but I think from a versioning point of view it's ok.

But the software is built for users, not versioning, isn't it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants