chore: refactor into es6 modules #17
Conversation
| typeAggregationFrequency = config[type].aggregationFrequency; | ||
| } | ||
| | ||
| this.tags = this.setTags(options.tags, config.tags, typeTags, config.app); |
There was a problem hiding this comment.
Can we rename this setSomething function to something like buildSomething or generateSomething?
I ask this because this doesn't look like methods that we use to set properties of the object and, instead, are more like pure functions that build a structured object using a set of inputs.
| | ||
| Object.assign(tags, globalTags); | ||
| Object.assign(tags, typeTags); | ||
| Object.assign(tags, methodTags); |
There was a problem hiding this comment.
Assigning the properties this way will probably overwrite some if they have the same key (for example methodTags['fizz'] will overwrite typeTags['fizz]). If that is intended and the properties are being overwritten in the correct order, it looks good 👍
| * @returns {*|Array} | ||
| */ | ||
| setAggregations(methodAggregations = [], globalAggregations = [], typeAggregations = []) { | ||
| let aggregations = globalAggregations; |
There was a problem hiding this comment.
Doing the aggregations attribution and concatenating like this may be changing the original globalAggregations because complex objects are passed as reference when passed as an argument to a function.
| * @returns {Array} | ||
| */ | ||
| filterAggregations(aggregations = []) { | ||
| return aggregations.filter((item) => { |
There was a problem hiding this comment.
This can be abbreviated to:
return aggregations.filter((item) => aggregationList.includes(item));| if (aggregationFrequencyList.includes(frequency)) { | ||
| freq = frequency; | ||
| } | ||
| |
There was a problem hiding this comment.
This can be abbreviated to:
let freq = (aggregationFrequencyList.includes(frequency)) ? frequency : 10;| clearMeasures: false | ||
| }; | ||
| | ||
| options = options || {}; |
There was a problem hiding this comment.
I think this is not needed because we're setting a default parameter to options
| | ||
| | ||
| Object.keys(options).forEach(function (key) { | ||
| Object.keys(options).forEach((key) => { |
There was a problem hiding this comment.
Can we use Object.assign here?
Object.assign(defaults, options);| logger.debug('Register Timer', metricName, metricValue, options); | ||
| this.logger.debug('Register Timer', metricName, metricValue, options); | ||
| if (metricName && metricValue >= 0) { | ||
| options = options || {}; |
There was a problem hiding this comment.
I think this is not needed because we are giving options a default value.
| metricValue = metricValue || 1; | ||
| | ||
| if (metricName) { | ||
| options = options || {}; |
There was a problem hiding this comment.
I think this is not needed because we are giving options a default value.
| try { | ||
| logger.debug('Register Counter', metricName, options); | ||
| this.logger.debug('Register Counter', metricName, options); | ||
| metricValue = metricValue || 1; |
There was a problem hiding this comment.
Can we give metricValue a default value?
No description provided.