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

Validate instrumentation key in Azure Exporters#789

Merged
lzchen merged 14 commits intocensus-instrumentation:masterfrom
lzchen:valid-ikey
Oct 23, 2019
Merged

Validate instrumentation key in Azure Exporters#789
lzchen merged 14 commits intocensus-instrumentation:masterfrom
lzchen:valid-ikey

Conversation

@lzchen
Copy link
Copy Markdown
Contributor

@lzchen lzchen commented Sep 18, 2019

Azure exporters will now throw an exception upon passing in an invalid instrumentation key.

The definition of a VALID instrumentation key is as follows:

  1. Not none
  2. Not empty
  3. Valid UUID according to this
    • Every character is a hex character [0-9a-f]
    • 32 characters are separated into 5 sections via 4 dashes
    • First section has 8 characters
    • Second section has 4 characters
    • Third section has 4 characters, the first one in [1-5]
    • Fourth section has 4 characters, the first one in [89ab]
    • Fifth section has 12 characters

@lzchen lzchen mentioned this pull request Sep 18, 2019
Merged
reyang
reyang reviewed Sep 18, 2019
View reviewed changes
reyang
reyang reviewed Sep 18, 2019
View reviewed changes
reyang
reyang reviewed Sep 18, 2019
View reviewed changes
reyang
reyang reviewed Sep 18, 2019
View reviewed changes
reyang
reyang suggested changes Sep 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Please find my comments.

hectorhdzg
hectorhdzg reviewed Oct 21, 2019
View reviewed changes
markwolff
markwolff reviewed Oct 21, 2019
View reviewed changes
@lzchen
Copy link
Copy Markdown
Contributor Author

lzchen commented Oct 22, 2019

@reyang
Can you take another look? I've made the changes that were suggested.

reyang
reyang reviewed Oct 23, 2019
View reviewed changes
contrib/opencensus-ext-azure/opencensus/ext/azure/common/utils.py
return urlparse(url).netloc


# Validate UUID format
Copy link
Copy Markdown
Contributor

@reyang reyang Oct 23, 2019

Choose a reason for hiding this comment

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

Why do we want UUID format link here?
We should have a link to the definition of "what's a valid instrumentation key for Azure Monitor".

Copy link
Copy Markdown
Contributor Author

@lzchen lzchen Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't believe there is an official definition of "what's a valid instrumentation key for Azure Monitor". The links that Mothra had referred to (with the prefix and suffix) do not seem to apply to the ikeys that are sent to Azure Monitor, so we decided that the validation would be to check for UUID. With that being said, I will supplement the links in the comments with more detail on explaining what it means to be a valid instrumentation key.

reyang
reyang reviewed Oct 23, 2019
View reviewed changes
reyang
reyang approved these changes Oct 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

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

I approve assuming all my comments will be properly addressed.

reyang
reyang reviewed Oct 23, 2019
View reviewed changes
@lzchen lzchen merged commit e9129b7 into census-instrumentation:master Oct 23, 2019
@lzchen lzchen mentioned this pull request Nov 18, 2019
Merged
@lzchen lzchen mentioned this pull request Dec 4, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@markwolff markwolff markwolff left review comments

@reyang reyang reyang approved these changes

@xiao-lix xiao-lix xiao-lix left review comments

@hectorhdzg hectorhdzg hectorhdzg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lzchen lzchen

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lzchen @markwolff @reyang @xiao-lix @hectorhdzg @googlebot