Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New configuration for globally controlled label value sizes #415

Merged
merged 1 commit into from
Feb 25, 2017
Merged

New configuration for globally controlled label value sizes #415

merged 1 commit into from
Feb 25, 2017

Conversation

matthewloring
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 23, 2017
@matthewloring matthewloring added the semver: major Hint for users that this is an API breaking change. label Feb 23, 2017
kjin
kjin approved these changes Feb 24, 2017
View reviewed changes
Copy link
Contributor

@kjin kjin left a comment

Choose a reason for hiding this comment

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

LGTM w/ comment.

src/span-data.js Outdated
@@ -71,7 +78,7 @@ function SpanData(agent, trace, name, parentSpanId, isRoot, skipFrames) {
callSite.getFileName(), callSite.getLineNumber(),
callSite.getColumnNumber()));
});
this.span.setLabel(TraceLabels.STACK_TRACE_DETAILS_KEY,
this.addLabel(TraceLabels.STACK_TRACE_DETAILS_KEY,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

kjin
kjin reviewed Feb 24, 2017
View reviewed changes
Copy link
Contributor

@kjin kjin left a comment

Choose a reason for hiding this comment

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

It may be good to add a test to ensure that stack traces are not truncated.

@matthewloring
Copy link
Contributor Author

Done.

ofrobots
ofrobots suggested changes Feb 24, 2017
View reviewed changes
config.js
databaseResultReportingSize: 127,
// The maximum number of characters reported on a label value. This
// cannot exceed 16383, the maximum value accepted by the service.
maximumLabelValueSize: 512,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/span-data.js Outdated
@@ -92,7 +97,10 @@ SpanData.prototype.createChildSpanData = function(name, skipFrames) {
};

SpanData.prototype.addLabel = function(key, value) {
this.span.setLabel(key, value);
key = traceUtil.truncate(key, constants.TRACE_SERVICE_LABEL_KEY_LIMIT);
var val = typeof value === 'object' ? util.inspect(value) : '' + value;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/span-data.js Outdated
@@ -72,7 +76,8 @@ function SpanData(agent, trace, name, parentSpanId, isRoot, skipFrames) {
callSite.getColumnNumber()));
});
this.span.setLabel(TraceLabels.STACK_TRACE_DETAILS_KEY,
JSON.stringify({stack_frame: stackFrames}));
traceUtil.truncate(JSON.stringify({stack_frame: stackFrames}),

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/test-util.js Outdated
});

it('should not truncate objects smaller than size', function() {
assert.equal(util.stringifyPrefix(o, 150),
'{a:5,b:hi,c:[Function],d:{e:{f:null,g:undefined,h:{0:1,1:2}}}}');
assert.equal(util.truncate('abcdefghijklmno', 50), 'abcdefghijklmno');

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@kjin kjin added this to the Beta milestone Feb 24, 2017
@matthewloring matthewloring merged commit eacfa15 into googleapis:master Feb 25, 2017
@matthewloring matthewloring deleted the span-shortening branch February 25, 2017 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrobots ofrobots ofrobots approved these changes

@kjin kjin kjin approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. semver: major Hint for users that this is an API breaking change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthewloring @ofrobots @kjin @googlebot