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: update usage collection to remove deprecated param #158

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

makeavish
Copy link
Member

@makeavish makeavish commented Jul 16, 2023

No description provided.

Also add ResourceTagsMap to trace model
@makeavish makeavish linked an issue Jul 16, 2023 that may be closed by this pull request
@makeavish makeavish marked this pull request as ready for review July 17, 2023 12:45
nityanandagohain

This comment was marked as off-topic.

Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

Let's calculate at the receiving level itself #157 (comment) ?

@makeavish makeavish merged commit 1b4097d into main Jul 18, 2023
3 checks passed
@makeavish makeavish deleted the fix/usage-collection branch July 18, 2023 04:52
@srikanthccv
Copy link
Member

Marshalling and JSON, especially, are very expensive in CPU resource usage. The seemingly simple change made it almost 2x what it used to be, and it will degrade the collector capability when the load is non-trivial. I would have looked for other approaches.

Side note: @nityanandagohain, it's good to write down why you went from requested changes to approved again without any comments. I am assuming you had some discussion which is fine but others wouldn't know anything about it.

@nityanandagohain
Copy link
Member

Yeah, so in the issue I suggested to calculate the size of the span at the receiving level itself i.e in the newStructuredSpan function, but since the ptrace.Span didn't had tags for marshalling Vishal suggested to keep this implementation itself.

@nityanandagohain
Copy link
Member

nityanandagohain commented Jul 18, 2023

@makeavish lets open a new issue so that we keep the the resource usage in account and make changes once we find a better solution to calculate size

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.

[Traces] Usge collection should happen at newStructuredSpan
3 participants