Skip to content

Conversation

steveww
Copy link
Contributor

@steveww steveww commented Feb 15, 2019

The type field for the SDK data object had the wrong case Type not type. The trace processing on the backend does a case sensitive lookup of this field. It does not find Type so tags the span as entry, the default. This makes it impossible to have an intermediate span, this fixes that.

Copy link

@mmanciop mmanciop left a comment

Choose a reason for hiding this comment

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

Nice catch! I am still not sure it affects in any way how we build calls (an entry span child of an entry span is treated as intermediate afaik), but this is indeed a bug :-)

Type = None
type = None
arguments = None
Return = None

Choose a reason for hiding this comment

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

Nice catch. Shouldn’t we do the same with Return? Also: I think we should fix it backend side too as this does not handle already compiled GoLang executables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I was so focused on the Type I missed Return. Just committed that too.
I'm just started to look at Golang to see if it has the same problems.
It would be quite a big change on the backend to make it case insensitive and could have performance implications due to the high volume of traces processed.

@pglombardo
Copy link
Contributor

pglombardo commented Feb 15, 2019

At one point there was a reason why we couldn't lowercase this field: 396a279

I'll take another look at this today. Thanks for the PR!

@steveww
Copy link
Contributor Author

steveww commented Feb 15, 2019 via email

@pglombardo
Copy link
Contributor

No - the backend has evolved and we haven't followed. Fixing this today. I'll might have to create another branch/PR to make sure we properly report k like we just did for Go as well.

@pglombardo
Copy link
Contributor

Ah I remember now :-) lowercase type and return are Python keywords. We'll have to lowercase in json encoding potentially. I'll see what magic I can pull...

@pglombardo
Copy link
Contributor

This PR is replaced by #132

Thanks for the PR @steveww!

@pglombardo pglombardo closed this Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants