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

Fix for non-null parentSpanId #66

Merged
merged 2 commits into from
Jun 29, 2018
Merged

Conversation

tcolgate
Copy link
Contributor

No description provided.

@tcolgate
Copy link
Contributor Author

I'm not sure on your preferred syntax here, but the parentId must be encoded if it exists.

@tcolgate tcolgate force-pushed the spanfix branch 2 times, most recently from 3b85cd6 to f62d08e Compare June 27, 2018 11:09
@tcolgate
Copy link
Contributor Author

At this point the failing tests seem to be in the stackdriver exporter (unrelated to this change)

@fabiogomessilva
Copy link
Member

Hi @tcolgate,

Thanks for your contribution.

Actually, it is just a tslint issue.

lerna ERR! test Errored while running script in '@opencensus/exporter-jaeger'
...
lerna ERR! clang-format reported errors... run `gts fix` to address.

Please run 'gts fix' at package/opencensus-exporter-jaeger, run the tests again and commit the changes.

@tcolgate
Copy link
Contributor Author

I might admit, I couldn't get the tests to run locally. They seem to fail, and I didn't see anything about gts in the output.

@fabiogomessilva
Copy link
Member

Your last commit fixed the issue. I suppose you had managed to run the tests, right?

@tcolgate
Copy link
Contributor Author

@fabiogomessilva no, but the gts fixed looked fine, I committed the result of that and let the CI tests run.
I'll try and get the tests working locally, but that may take me a while (node isn't my primary dev environment).

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, thanks for the fix. At this point the CI failure seems to be related to degraded npm package download service in CircleCI. I'll re-trigger the run and see how it goes.

@tcolgate
Copy link
Contributor Author

Is there a readme for setting up the dev environment?

@tcolgate tcolgate force-pushed the spanfix branch 2 times, most recently from 1f2853e to 8e3b289 Compare June 28, 2018 10:50
@tcolgate
Copy link
Contributor Author

@kjin I've added a test to check spans encode to the thrift spans. Seemed worth while, and forced me to get the local tests working.

@kjin
Copy link
Contributor

kjin commented Jun 28, 2018

@tcolgate Thanks for the extra test.

The hope is that setting up a dev environment is nothing more than npm install (with re-linking done between packages being lerna bootstrap). That probably is information that should go into a developer's guide, thanks for pointing that out. However, that should be the extent of "setting up" -- please feel free to file an issue if that is not working.

@kjin
Copy link
Contributor

kjin commented Jun 29, 2018

@tcolgate I just rebased since the Node 6 CI doesn't seem to want to work otherwise (still the same issue as I mentioned earlier, though it appears not to be affecting other PRs for some reason). If this passes I will land it.

@kjin kjin merged commit 462be4e into census-instrumentation:master Jun 29, 2018
kjin pushed a commit that referenced this pull request Jun 29, 2018
* fix: thrift encoding of non-null parentSpanId fails

* test: add test for span encoding
kjin pushed a commit to kjin/opencensus-node that referenced this pull request Aug 24, 2018
* fix: thrift encoding of non-null parentSpanId fails

* test: add test for span encoding
kjin pushed a commit to kjin/opencensus-node that referenced this pull request Aug 24, 2018
* fix: thrift encoding of non-null parentSpanId fails

* test: add test for span encoding
kjin pushed a commit to kjin/opencensus-node that referenced this pull request Aug 24, 2018
* fix: thrift encoding of non-null parentSpanId fails

* test: add test for span encoding
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants