Closed Bug 1438561 Opened 6 years ago Closed 6 years ago

Restrict histogram bucket ranges to INT_MAX

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: gfritzsche, Assigned: is2ei, Mentored)

References

Details

(Whiteboard: [good first bug][lang=python])

Attachments

(1 file, 3 obsolete files)

In bug 1438335, we clarify that our supported recording range for values into histograms is 0 to INT_MAX.

We can catch any usage of larger buckets at build time in our python scripts and present a clear error to the user.
parse_histograms.py does check if "high" is a Python `int`... but `int` in Python is implemented[1] as a long in C, which can be anything it wants to be, so long as it's at least 32-bit. (search terms include ILP64, LLP64, and 'why C++, why?')

Setting this up as a mentored bug...

[1]: https://docs.python.org/2/library/stdtypes.html#numeric-types-int-float-long-complex
To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. You'll be working in parse_histograms.py which you can find in toolkit/components/telemetry. 
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days.
3) Build your change with `mach build` and test your change with `mach test toolkit/components/telemetry/tests/`. Also check your changes for adherence to our style guidelines by using `mach lint`
4) Submit the patch (including an automated test, similar to the ones in toolkit/components/telemetry/tests/python) for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.
Mentor: chutten
Priority: -- → P3
Whiteboard: [good first bug][lang=python]
Hey Chris I'd like to be assigned this bug.  It is my first one.
Hi, I would like to work on this bug.
mack7121, I have assigned it to you. If you have any questions, please feel free to ask here as mentioned under Step 2.
Assignee: nobody → mack7121
Status: NEW → ASSIGNED
Thank you, I will get started on it.
mack7121, do you need any help? Are you working on this?
Flags: needinfo?(mack7121)
So, just a couple of pointers on how to go about solving this bug. We need to calculate the range of values that the bucket accepts (using the 'low', 'high' and 'n_buckets' values for the histogram as a whole) and ensure that: low_bucket < INT_MAX, high_bucket < INT_MAX for every bucket, (where low_bucket and high_bucket are calculated, not read in from the histogram properties). Note that it's a _strict_ inequality, as values >= INT_MAX are accumulated as INT_MAX - 1.

You may want to keep in mind is that the last bucket is an overflow bucket, so that is not counted when finding range per bucket.

Also, regarding the platform dependent size of INT_MAX...

(In reply to Chris H-C :chutten from comment #1)
> parse_histograms.py does check if "high" is a Python `int`... but `int` in
> Python is implemented[1] as a long in C, which can be anything it wants to
> be, so long as it's at least 32-bit. (search terms include ILP64, LLP64, and
> 'why C++, why?')
> 
> Setting this up as a mentored bug...
> 
> [1]:
> https://docs.python.org/2/library/stdtypes.html#numeric-types-int-float-long-complex

I think the python ctypes library will be immensely useful here. I don't know if it gives the limits, but it definitely gives the size in bytes of various c data types (int, uint, etc..) on that platform. Take a look at the ctypes.sizeof() function and the ctypes documentation.

So you can skip the technical LLP64, ILP64 related queries. :)
https://docs.python.org/2.7/library/ctypes.html#ctypes.c_int
Assignee: mack7121 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mack7121)
Hi :) Is this bug part of the Outreachy internship?
(In reply to Khushi Sharma from comment #9)
> Hi :) Is this bug part of the Outreachy internship?

I don't know that there are any bugs that are specifically part of or not part of Outreachy internships. 

This bug is available to be worked on if you're looking for something like this to work on.
I would like to work on this.
Excellent. I have assigned this bug to you, blackbess. Please let me know if you have any questions.
Assignee: nobody → reddyrushi20
Status: NEW → ASSIGNED
:blackbess are you still working on this bug? Do you need some help?
Flags: needinfo?(reddyrushi20)
Unassigning due to inactivity. If this is in error, just drop a comment and I'll reassign it back.
Assignee: reddyrushi20 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(reddyrushi20)
Hi, Chris
Could I work on this issue if this is still open? :)
Flags: needinfo?(chutten)
Certainly! Let me know if you have any questions.
Assignee: nobody → is2ei.horie
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)
Thanks Chris!

I encountered this error when I run test command without any modification of the source code.

===========
$ ./mach test toolkit/components/telemetry/tests/
cat: backend.TestManifestBackend.in: No such file or directory
Build configuration changed. Regenerating backend.
Traceback (most recent call last):
  File "/Users/horie/oss/mozilla/mozilla-unified/build/gen_test_backend.py", line 39, in <module>
    sys.exit(gen_test_backend())
  File "/Users/horie/oss/mozilla/mozilla-unified/build/gen_test_backend.py", line 35, in gen_test_backend
    backend.consume(data)
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mozbuild/mozbuild/backend/base.py", line 126, in consume
    for obj in objs:
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mozbuild/mozbuild/frontend/emitter.py", line 172, in emit
    for out in output:
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mozbuild/mozbuild/frontend/reader.py", line 899, in read_topsrcdir
    for r in self.read_mozbuild(path, self.config):
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mozbuild/mozbuild/frontend/reader.py", line 1073, in read_mozbuild
    sys.exc_info()[2], sandbox_load_error=sle)
mozbuild.frontend.reader.BuildReaderError: 
==============================
FATAL ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /Users/horie/oss/mozilla-unified/moz.build

The underlying problem is we referenced a path that does not exist. That path is:

    /Users/horie/oss/mozilla-unified/moz.build

Either create the file if it needs to exist or do not reference it.

make: *** [backend.TestManifestBackend] Error 1
Error running mach:

    ['test', 'toolkit/components/telemetry/tests/']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

Exception: Process executed with non-0 exit code 2: [u'/usr/bin/make', u'-f', u'/Users/horie/oss/mozilla/mozilla-unified/build/rebuild-backend.mk', u'-j8', u'-s', u'backend.TestManifestBackend']

  File "/Users/horie/oss/mozilla/mozilla-unified/testing/mach_commands.py", line 117, in test
    resolver = self._spawn(TestResolver)
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mozbuild/mozbuild/base.py", line 741, in _spawn
    topobjdir=self.topobjdir)
  File "/Users/horie/oss/mozilla/mozilla-unified/testing/mozbase/moztest/moztest/resolve.py", line 401, in __init__
    self.topsrcdir, 'build', 'gen_test_backend.py'),
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mozbuild/mozbuild/base.py", line 669, in _run_make
    return fn(**params)
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mozbuild/mozbuild/base.py", line 724, in _run_command_in_objdir
    return self.run_process(cwd=self.topobjdir, **args)
  File "/Users/horie/oss/mozilla/mozilla-unified/python/mach/mach/mixin/process.py", line 153, in run_process
    raise Exception('Process executed with non-0 exit code %d: %s' % (status, args))
================

Could you help me to fix this issue?
Flags: needinfo?(chutten)
Oooh, that's a new one for me. It's complaining that you don't have a moz.build in your root mozilla-unified dir. You should have one, and it should look like this: https://searchfox.org/mozilla-central/source/moz.build -- maybe update your local checkout to be up-to-date?

I'm not sure what situation might have caused you not to have a moz.build. Maybe ask on #introduction on IRC? They've probably seen this before and can recommend next steps
Flags: needinfo?(chutten)
Thanks Chris,

I can run "./mach test toolkit/components/telemetry/tests/" now!
I will continue to work on the issue.
Attached patch bug-1438561.patch (obsolete) — Splinter Review
Hi Chris,

I attached a patch.
Could you please check it? Thanks.
Attachment #8971851 - Flags: review?(chutten)
Comment on attachment 8971851 [details] [diff] [review]
bug-1438561.patch

Review of attachment 8971851 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thank you. Can we add a test for it?
Attachment #8971851 - Flags: review?(chutten) → review+
Attached patch bug-1438561.patch (obsolete) — Splinter Review
Thanks Chris!

I added a test. Could you please review it?
Attachment #8971851 - Attachment is obsolete: true
Attachment #8972297 - Flags: review?(chutten)
Comment on attachment 8972297 [details] [diff] [review]
bug-1438561.patch

Review of attachment 8972297 [details] [diff] [review]:
-----------------------------------------------------------------

Nice test! I have one concern about using python system limits to hit a C compiler limit. Maybe it'd be best to just use a number like 2**64.

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py
@@ +130,5 @@
> +                "bug_numbers": [1383793],
> +                "expires_in_version": "never",
> +                "kind": "exponential",
> +                "low": 1024,
> +                "high": sys.maxint,

On 32-bit platforms, couldn't sys.maxint be within the range of c_int? (Python2 docs only guarantee that it's at least 2^31 - 1)
Attachment #8972297 - Flags: review?(chutten)
Attached patch bug-1438561.patch (obsolete) — Splinter Review
Thanks!
I changed it not to use sys.maxint .
Could you check it, please?
Attachment #8972473 - Flags: review?(chutten)
Thanks!
I changed it not to use sys.maxint .
Could you check it, please?
Attachment #8972297 - Attachment is obsolete: true
Attachment #8972473 - Attachment is obsolete: true
Attachment #8972473 - Flags: review?(chutten)
Attachment #8972474 - Flags: review?(chutten)
Ah, I'm sorry those my comments are duplicated.
Comment on attachment 8972474 [details] [diff] [review]
bug-1438561.patch

Review of attachment 8972474 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!
Attachment #8972474 - Flags: review?(chutten) → review+
There's a soft code freeze in place for Firefox 61 as we approach merge day on the seventh. To be kind, we should probably hold of marking this checkin-needed until Monday. 

So I'll be setting ni? for myself so I don't forget to take care of that, and then you can consider this bug fixed! I see you've already found a bug or two to work on next, and that's great.
Flags: needinfo?(chutten)
Ok. Thanks!
Flags: needinfo?(chutten)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9296d2a3287
Restrict histogram bucket ranges to INT_MAX. r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9296d2a3287
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: