chore: add type definitions for exporter-jaeger by justindsmith · Pull Request #93 · census-instrumentation/opencensus-node

justindsmith · 2018-08-12T03:23:27Z

The typescript definition files weren't being generated for the exporter-jaeger package because the tsconfig didn't extend from the gts base config.

googlebot · 2018-08-12T03:23:33Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

justindsmith · 2018-08-15T02:18:03Z

CLA has been signed!

kjin

Thanks for the PR!

kjin · 2018-08-15T17:21:10Z

+};
+
+export type ThriftSpan = {
+  traceIdLow: any,    // tslint:disable-line:no-any


Are the any fields actually any? I feel like they can be strings instead for the most part.

kjin · 2018-08-15T17:22:02Z

    parentSpanId: parentSpan,
    operationName: span.name,
-    references: [],
+    references: [] as any,  // tslint:disable-line:no-any


Should be casted to whatever the type of references is, currently any[] but hopefully a tighter type.

Yeah, let me dig in and see if I can tighten up the types.

The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.

justindsmith · 2018-08-15T20:59:35Z

Added types for all the ThriftSpan properties.

In doing more testing I found that the JaegerTraceExporterOptions tags weren't being processed correctly as a Tag[] but were handled correctly as a Record<string, TagValue>. I'm making the assumption that the Tag[] was the proper type, so I fixed processing to support that.

Here's what I added in the commit message. I wanted to make sure it was highlighted since it's potentially breaking api:

The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was
processing those tags for the process information as if they were an object /
Record instead of a Tag array. This was leading to the process key-value
inside Jaeger to be the array index as key and the value as the key-value
object. We now use the proper key-value information from the input to place
into the process tags.

There were two approaches we could have taken here: 1. To leave the input
as a Tag array and process the input properly in the code or 2. To change
the input to a key-value record / object (Record<string, TagValue), which
would be handled properly by the existing code. We have chosen approach #1
because a Jaeger process tag CAN support multiple tags with the same key
name, which would not be supported in approach #2. There is a chance that
(because we were not previously exporting typescript types) users were
passing in a Record format instead of Array, and thus this might be seen
as a breaking change.

kjin

LGTM w/ nit. I'll make sure to include your comments in the squashed commit message.

kjin · 2018-08-15T21:13:59Z

+          return;
+        }
+      });
+      assert.strictEqual(


You can just use assert.ok here.

kjin · 2018-08-16T16:59:38Z

Hi @justindsmith -- This is good to land but I couldn't find your name in the CLA database. Could you retry the CLA signing procedure please?

justindsmith · 2018-08-16T17:20:20Z

@kjin I kicked off the process to submit on behalf of my employer (Omnition) and not individually. I thought we had done all the right steps (setup internal google group, add relevant users to that list, get it signed, etc). Is there a separate directory for employer signed contributors?

(I mostly want to make sure that we setup that process correctly for other future contributors from our team.)

kjin · 2018-08-16T17:24:48Z

I see. I don't see it in the list right now, but it seems like it might take some time (a few days -- so hopefully today). I will check back periodically... thanks for the clarification.

justindsmith · 2018-08-16T17:27:02Z

Ok great. Thanks. We signed it this past Tuesday morning just for reference. If it doesn't show up we can follow up to see what might be going on.

googlebot · 2018-08-17T19:04:37Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

kjin · 2018-08-17T19:05:22Z

@justindsmith Found Omnition in the database, so we're good to go 👍

…on#93) The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.

chore: add type definitions for exporter-jaeger

5f54894

googlebot added the cla: no label Aug 12, 2018

refactor: add and refactor jaeger and thrift types

65272c7

kjin reviewed Aug 15, 2018

View reviewed changes

kjin approved these changes Aug 15, 2018

View reviewed changes

test: change assert.strictEqual to assert.ok where appropriate

0b65ccc

kjin added the cla: yes label Aug 17, 2018

googlebot removed the cla: no label Aug 17, 2018

kjin merged commit 5882009 into census-instrumentation:master Aug 17, 2018

justindsmith deleted the jaeger-exporter-types branch August 17, 2018 19:21

Conversation

justindsmith commented Aug 12, 2018

Uh oh!

googlebot commented Aug 12, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

justindsmith commented Aug 15, 2018

Uh oh!

kjin left a comment

Choose a reason for hiding this comment

Uh oh!

kjin Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

kjin Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

justindsmith Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

justindsmith commented Aug 15, 2018

Uh oh!

kjin left a comment

Choose a reason for hiding this comment

Uh oh!

kjin Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

kjin commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justindsmith commented Aug 16, 2018

Uh oh!

kjin commented Aug 16, 2018

Uh oh!

justindsmith commented Aug 16, 2018

Uh oh!

googlebot commented Aug 17, 2018

Uh oh!

kjin commented Aug 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kjin commented Aug 16, 2018 •

edited

Loading