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.

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

Fix trace state parsing. #638

Merged

Conversation

NTaylorMullen
Copy link
Contributor

  • Without this the .Value<string> was exploding attempting to convert to a string.

- Without this the `.Value<string>` was exploding attempting to convert to a string.
@github-actions github-actions bot added this to the v0.19.3 milestone Aug 17, 2021
@NTaylorMullen
Copy link
Contributor Author

NTaylorMullen commented Aug 17, 2021

/cc @david-driscoll @ToddGrun

@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #638 (9bc65b7) into master (5281160) will decrease coverage by 3.52%.
The diff coverage is 75.00%.

❗ Current head 9bc65b7 differs from pull request most recent head 9eec18e. Consider uploading reports for the commit 9eec18e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
- Coverage   72.89%   69.37%   -3.53%     
==========================================
  Files         257      257              
  Lines       12576    12580       +4     
  Branches      848      848              
==========================================
- Hits         9167     8727     -440     
- Misses       3409     3583     +174     
- Partials        0      270     +270     
Impacted Files Coverage Δ
src/JsonRpc/Receiver.cs 90.27% <75.00%> (-6.79%) ⬇️
src/JsonRpc/ExternalServiceProvider.cs 50.00% <0.00%> (-50.00%) ⬇️
src/JsonRpc.Generators/SymbolExtensions.cs 71.42% <0.00%> (-28.58%) ⬇️
src/JsonRpc/Server/ContentModifiedException.cs 37.50% <0.00%> (-18.75%) ⬇️
src/JsonRpc/Server/RequestCancelledException.cs 37.50% <0.00%> (-18.75%) ⬇️
...rver/Configuration/ChainedConfigurationProvider.cs 58.62% <0.00%> (-17.25%) ⬇️
...ient/Configuration/ChainedConfigurationProvider.cs 0.00% <0.00%> (-17.25%) ⬇️
...rver/Configuration/ChainedConfigurationProvider.cs 0.00% <0.00%> (-17.25%) ⬇️
src/JsonRpc/Server/ServerErrorResult.cs 83.33% <0.00%> (-16.67%) ⬇️
src/JsonRpc.Generators/Contexts/SyntaxSymbol.cs 75.00% <0.00%> (-16.67%) ⬇️
... and 97 more

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 5281160...9eec18e. Read the comment docs.

@andyleejordan
Copy link
Contributor

@NTaylorMullen just out of curiosity, can I see how this was failing for you? In case something similar crops up in PowerShell Editor Services 😆

@NTaylorMullen
Copy link
Contributor Author

@NTaylorMullen just out of curiosity, can I see how this was failing for you? In case something similar crops up in PowerShell Editor Services 😆

I have it all patched locally so not easy to reproduce but it was a Cast exception. The gist is that when you got a jsonrpc payload which had:

{
    "traceparent": "alskdfj-asldkjf-asdj-asdflkj-sadfjasd-fj"
    ....
}

If you have the powershell language server running in VS you'll see the exception too (they provide tracing); although I don't believe VSCode's jsonrpc bits do.

@andyleejordan
Copy link
Contributor

Oh interesting, not sure I was using traceparent yet but I remember seeing it get enabled.

@andyleejordan
Copy link
Contributor

Ah yes this PR: #553

@andyleejordan
Copy link
Contributor

Taylor, do you think your PR will fix #609 too?

@david-driscoll david-driscoll merged commit b588faa into OmniSharp:master Aug 18, 2021
@github-actions github-actions bot added the mysterious We forgot to label this label Aug 18, 2021
@NTaylorMullen
Copy link
Contributor Author

Taylor, do you think your PR will fix #609 too?

Yup, absolutely will fix that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mysterious We forgot to label this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants