Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Add validation for tagkey & tagvalue. Fix issue #268#280

Merged
mayurkale22 merged 6 commits intocensus-instrumentation:masterfrom
vigneshtdev:add-validation-for-tags
Jan 16, 2019
Merged

Add validation for tagkey & tagvalue. Fix issue #268#280
mayurkale22 merged 6 commits intocensus-instrumentation:masterfrom
vigneshtdev:add-validation-for-tags

Conversation

@vigneshtdev
Copy link
Copy Markdown
Contributor

Validates tagkey & tagvalues as per the specification given in https://github.com/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md#tagkey
ran npm test & npm run complie with no errors.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 13, 2019

Codecov Report

Merging #280 into master will decrease coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
- Coverage   94.81%   94.57%   -0.24%     
==========================================
  Files         110      106       -4     
  Lines        7771     7434     -337     
  Branches      714      694      -20     
==========================================
- Hits         7368     7031     -337     
  Misses        403      403
Impacted Files Coverage Δ
test/test-stackdriver-monitoring.ts 97.1% <0%> (-0.26%) ⬇️
src/metrics/gauges/derived-gauge.ts 96.42% <0%> (-0.07%) ⬇️
src/metrics/gauges/gauge.ts 100% <0%> (ø) ⬆️
test/test-console-logger.ts 100% <0%> (ø) ⬆️
test/test-derived-gauge.ts 100% <0%> (ø) ⬆️
test/test-metric-producer.ts 100% <0%> (ø) ⬆️
test/test-view.ts 99.2% <0%> (ø) ⬆️
test/test-gauge.ts 100% <0%> (ø) ⬆️
src/metrics/export/metric-producer-manager.ts 100% <0%> (ø) ⬆️
test/test-metric-registry.ts 100% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 239a9e2...0bc2a1d. Read the comment docs.

* @param tags The tags to be checked
*/
private invalidTags(tags: Tags): boolean {
return this.invalidPrintableCharacters(tags) || this.invalidLength(tags);
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.

I think we should log the warn or error, in case of invalid tags. WDYT?

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.

How about modifying the method like this:

private invalidTags(tags: Tags): boolean {
  const result: boolean =  this.invalidPrintableCharacters(tags) || this.invalidLength(tags);
  if(result) {
    console.log('Unable to create tagkey/tagvalue with the specified tags. Check Tag specifications here: https://github.com/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md');
  }
  return result;
}

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.

s/console.log/this.logger.warn and no need to add specs link.

Comment thread packages/opencensus-core/test/test-view.ts Outdated
@mayurkale22
Copy link
Copy Markdown
Member

Thanks @vigneshtdev for contribution... 👍

@mayurkale22 mayurkale22 merged commit f1983b9 into census-instrumentation:master Jan 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants