Correctly serialize lock statistics in system.profile. #144

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@einaregilsson
Contributor

einaregilsson commented Dec 10, 2012

Problems:

  1. The SystemProfileInfoSerializer used the name "lockStatMillis" for the lock statistics key name. At least in MongoDB 2.2 the correct name is "lockStats"
  2. The SystemProfileLockStatisticsSerializer used the names "timeAcquiring" and "timeLocked" where the right names are "timeAcquiringMicros" and "timeLockedMicros".
  3. The timespan units used were milliseconds, where the correct units are microseconds. Added the option MicroSeconds to the TimeSpanSerializer and set the SystemProfileReadWriteLockStatisticsSerializer to use Microseconds.
  4. NOT FIXED: The TimeSpanUnits.Nanoseconds is wrong. 1 millisecond is not 1000 nanoseconds, its 1000000 nanoseconds. This option should probably be removed since TimeSpan doesn't have nanosecond precision (most accurate precision is ticks, where 10000 ticks are one millisecond).
Correctly serialize lock statistics in system.profile.
Problems:

 1. The SystemProfileInfoSerializer used the name "lockStatMillis" for the lock statistics key name. At least in MongoDB 2.2 the correct name is "lockStats"

 2. The SystemProfileLockStatisticsSerializer used the names "timeAcquiring" and "timeLocked" where the right names are "timeAcquiringMicros" and "timeLockedMicros".

 3. The timespan units used were milliseconds, where the correct units are microseconds. Added the option MicroSeconds to the TimeSpanSerializer and set the SystemProfileReadWriteLockStatisticsSerializer to use Microseconds.

 4. NOT FIXED: The TimeSpanUnits.Nanoseconds is wrong. 1 millisecond is not 1000 nanoseconds, its 1000000 nanoseconds. This option should probably be removed since TimeSpan doesn't have nanosecond precision (most accurate precision is ticks, where 10000 ticks are one millisecond).
@rstam

This comment has been minimized.

Show comment
Hide comment
@rstam

rstam Dec 12, 2012

Contributor

Created a JIRA ticket for this: https://jira.mongodb.org/browse/CSHARP-646

Contributor

rstam commented Dec 12, 2012

Created a JIRA ticket for this: https://jira.mongodb.org/browse/CSHARP-646

@rstam

This comment has been minimized.

Show comment
Hide comment
@rstam

rstam Dec 12, 2012

Contributor

That's really interesting (and surprising) that all of the TimeSpan.FromSomething methods truncate the result to a precision of milliseconds. Thanks for figuring that out!

I think it's OK to have a TimeSpanUnits of Nanoseconds even though the precision of TimeSpans is not that precise. The point of the units is to allow interoperating with interval values from other systems as well, and we want to be able to read TimeSpans from the database even if they happen to be expressed in nanoseconds (knowing that we are going to lose a bit of precision in the process).

FYI, the documentation on the format of the documents in system.profile is pretty scanty. Turns out there are other fields that we didn't know about either, so I'm attempting to get a full definition of this document format and address those as well.

Contributor

rstam commented Dec 12, 2012

That's really interesting (and surprising) that all of the TimeSpan.FromSomething methods truncate the result to a precision of milliseconds. Thanks for figuring that out!

I think it's OK to have a TimeSpanUnits of Nanoseconds even though the precision of TimeSpans is not that precise. The point of the units is to allow interoperating with interval values from other systems as well, and we want to be able to read TimeSpans from the database even if they happen to be expressed in nanoseconds (knowing that we are going to lose a bit of precision in the process).

FYI, the documentation on the format of the documents in system.profile is pretty scanty. Turns out there are other fields that we didn't know about either, so I'm attempting to get a full definition of this document format and address those as well.

@einaregilsson

This comment has been minimized.

Show comment
Hide comment
@einaregilsson

einaregilsson Dec 13, 2012

Contributor

It might make sense to split this up into two tickets, since the nanosecond thing doesn't really have anything to do with the system.profile stuff, it's just something I noticed.

Nanosecond issue

If we forget about the precision issue for a moment, there's still the issue that 1 millisecond = 1.000.000 nanoseconds, but the serializer uses 1 milliseconds = 1.000 nanoseconds. So if you have timespans stored in nanoseconds in the database they will be incorrectly deserialized into a TimeSpan.

About the precision, you can get precision down to a 100 nanoseconds in a TimeSpan, but it depends on which .FromX method you use to initalize the TimeSpan. The timespan uses Ticks where 1 tick = 100 nanoseconds.

Console.WriteLine(TimeSpan.FromMilliseconds(0.5).TotalMilliseconds); // writes 1 milliseconds
Console.WriteLine(TimeSpan.FromMilliseconds(0.4).TotalMilliseconds); // writes 0 milliseconds
Console.WriteLine(TimeSpan.FromTicks(5000).TotalMilliseconds);          // writes 0.5 milliseconds

So to get the TimeSpan with the best possible precision (100 nanosecond) from nanoseconds you could do

long nanoseconds = 544;
var ts = TimeSpan.FromTicks(nanoseconds / 100)

Fixing the 1000 vs 1000000 issue is going to be a breaking change no matter what. I would argue that it would be better to just remove .Nanoseconds as an option, and let people use longs directly if they are storing nanoseconds. But it's not something I care deeply about, just something I noticed and thought was weird :)

system.profile issue

I found that in commit 571b5d974f the names were changed (and perhaps the units as well? Don't know if they were always microseconds or if they actually were milliseconds before). I'm guessing that change first came out in 2.2 ?

That makes it a bit tricky to figure out what the right thing to do here is. Go with the new names and drop compatibility with the older versions? Somehow handle both names? We also have an old 1.6 server lying around, and there the system.profile looks completely different, so it seems like the system.profile object is a bit of a moving target to have a concrete class for.

Contributor

einaregilsson commented Dec 13, 2012

It might make sense to split this up into two tickets, since the nanosecond thing doesn't really have anything to do with the system.profile stuff, it's just something I noticed.

Nanosecond issue

If we forget about the precision issue for a moment, there's still the issue that 1 millisecond = 1.000.000 nanoseconds, but the serializer uses 1 milliseconds = 1.000 nanoseconds. So if you have timespans stored in nanoseconds in the database they will be incorrectly deserialized into a TimeSpan.

About the precision, you can get precision down to a 100 nanoseconds in a TimeSpan, but it depends on which .FromX method you use to initalize the TimeSpan. The timespan uses Ticks where 1 tick = 100 nanoseconds.

Console.WriteLine(TimeSpan.FromMilliseconds(0.5).TotalMilliseconds); // writes 1 milliseconds
Console.WriteLine(TimeSpan.FromMilliseconds(0.4).TotalMilliseconds); // writes 0 milliseconds
Console.WriteLine(TimeSpan.FromTicks(5000).TotalMilliseconds);          // writes 0.5 milliseconds

So to get the TimeSpan with the best possible precision (100 nanosecond) from nanoseconds you could do

long nanoseconds = 544;
var ts = TimeSpan.FromTicks(nanoseconds / 100)

Fixing the 1000 vs 1000000 issue is going to be a breaking change no matter what. I would argue that it would be better to just remove .Nanoseconds as an option, and let people use longs directly if they are storing nanoseconds. But it's not something I care deeply about, just something I noticed and thought was weird :)

system.profile issue

I found that in commit 571b5d974f the names were changed (and perhaps the units as well? Don't know if they were always microseconds or if they actually were milliseconds before). I'm guessing that change first came out in 2.2 ?

That makes it a bit tricky to figure out what the right thing to do here is. Go with the new names and drop compatibility with the older versions? Somehow handle both names? We also have an old 1.6 server lying around, and there the system.profile looks completely different, so it seems like the system.profile object is a bit of a moving target to have a concrete class for.

@rstam

This comment has been minimized.

Show comment
Hide comment
@rstam

rstam Dec 13, 2012

Contributor

I've created a new JIRA ticket to separate out the changes to TimeSpanSerializer:

https://jira.mongodb.org/browse/CSHARP-647

Contributor

rstam commented Dec 13, 2012

I've created a new JIRA ticket to separate out the changes to TimeSpanSerializer:

https://jira.mongodb.org/browse/CSHARP-647

@rstam

This comment has been minimized.

Show comment
Hide comment
@rstam

rstam Dec 13, 2012

Contributor

I merged in your pull request as is, though we do plan on doing some further work on top of this per CSHARP-646 and CSHARP-647.

Many thanks!

Contributor

rstam commented Dec 13, 2012

I merged in your pull request as is, though we do plan on doing some further work on top of this per CSHARP-646 and CSHARP-647.

Many thanks!

@rstam rstam closed this Dec 13, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment