Skip to content
This repository has been archived by the owner on Apr 8, 2019. It is now read-only.

Move opentracing util to m3x #214

Merged
merged 2 commits into from
Apr 2, 2019
Merged

Conversation

benraskin92
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Mar 31, 2019

Codecov Report

Merging #214 into master will decrease coverage by <.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
- Coverage   78.32%   78.31%   -0.01%     
==========================================
  Files          87       88       +1     
  Lines        3469     3487      +18     
==========================================
+ Hits         2717     2731      +14     
- Misses        684      688       +4     
  Partials       68       68

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 8f6e378...e0344ee. Read the comment docs.

@benraskin92 benraskin92 marked this pull request as ready for review March 31, 2019 20:10
@benraskin92 benraskin92 changed the title [WIP] Move opentracing util to m3x Move opentracing util to m3x Apr 1, 2019
@benraskin92 benraskin92 requested a review from arnikola April 1, 2019 15:38
@benraskin92 benraskin92 force-pushed the braskin/move_opentracing branch from 49133f0 to 443401e Compare April 1, 2019 20:05
Copy link
Contributor

@arnikola arnikola left a comment

Choose a reason for hiding this comment

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

LGTM, may be worth having Andrew give it a quick scan though

@@ -18,7 +18,7 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

package instrument
package opentracingutil
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rename so as to keep the package name the same as the folder name.


"github.com/opentracing/opentracing-go"
"github.com/opentracing/opentracing-go/log"
"golang.org/x/net/context"
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to prefer this over the standard lib "context" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope - I'll switch.

return span, opentracing.ContextWithSpan(ctx, span)
}

// Time is a log.Field for time.Time values. It translates to RF3339 formatted time strings.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: RFC3339. Maybe also add an example of what that means, e.g. "2019-04-01T15:04:05Z07:00"

@benraskin92 benraskin92 merged commit 8954de4 into master Apr 2, 2019
@benraskin92 benraskin92 deleted the braskin/move_opentracing branch April 2, 2019 14:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants