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

Fixed wrong time calc in dotnetspy. #252

Merged
merged 4 commits into from
Jun 22, 2021
Merged

Fixed wrong time calc in dotnetspy. #252

merged 4 commits into from
Jun 22, 2021

Conversation

AdrK
Copy link
Contributor

@AdrK AdrK commented Jun 21, 2021

No description provided.

@@ -89,7 +89,7 @@ func (s *session) start() error {
for k, v := range p.Samples() {
s.ch <- line{
name: []byte(k),
val: int(v.Milliseconds()) / 10,
val: int(v.Milliseconds()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The examples/dotnet/web was not working and I found that this is due to wrong calculations here.
I wonder why we were doing /10 ? For v.Milliseconds() == 1, this was resolving to 0 and no samples were reported in stacktrace.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a leftover. Thank you for fixing this!

@kolesnikovae
Copy link
Collaborator

LGTM.

I've also updated dotnetdiag package - could you please bump its version to v1.2.1?

@petethepig petethepig marked this pull request as ready for review June 22, 2021 02:46
@petethepig
Copy link
Member

@kolesnikovae So that must mean we should be seeing more data now when we profile dotnet apps, right?

@codecov
Copy link

codecov bot commented Jun 22, 2021

Codecov Report

Merging #252 (7144e59) into main (2723d11) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
+ Coverage   54.18%   54.20%   +0.03%     
==========================================
  Files          87       87              
  Lines        3631     3631              
==========================================
+ Hits         1967     1968       +1     
+ Misses       1465     1464       -1     
  Partials      199      199              
Impacted Files Coverage Δ
pkg/agent/upstream/remote/remote.go 62.32% <0.00%> (-2.89%) ⬇️
pkg/agent/session.go 66.40% <0.00%> (+2.46%) ⬆️

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 2723d11...7144e59. Read the comment docs.

@petethepig petethepig merged commit f1dff48 into main Jun 22, 2021
@petethepig petethepig deleted the dotnetspy_fix branch June 22, 2021 02:55
@kolesnikovae
Copy link
Collaborator

kolesnikovae commented Jun 22, 2021

My bad, this denominator is not a mistake - #255:

// dotnet profiler reports total time v per call stack k.
// Meanwhile, pyroscope agent expects number of samples is
// reported. Every sample is a time fraction of second
// according to sample rate: 1000ms/100 = 10ms by default.
// To represent reported time v as a number of samples,
// we divide it by sample duration.
//
// Taking into account that under the hood dotnet spy uses
// Microsoft-DotNETCore-SampleProfiler, which captures a
// snapshot of each thread's managed callstack every 10 ms,
// we cannot manage sample rate from outside.

Thus, we cannot capture samples of duration less than 1s/sample-rate (10ms by default).

@AdrK, the reason why web example didn't show anything is that we filter out "unmanaged" code (native): unless there are no incoming requests, collected profiles will be empty (though, sometimes on slow machines we can capture time spent in managed code that invokes native methods).

korniltsev pushed a commit that referenced this pull request Jul 18, 2023
 Reexport values from helm into a JSON file
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.

None yet

3 participants