Closed
Bug 1438561
Opened 6 years ago
Closed 6 years ago
Restrict histogram bucket ranges to INT_MAX
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
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)
2.87 KB,
patch
|
chutten
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•6 years ago
|
||
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
Comment 2•6 years ago
|
||
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]
Comment 4•6 years ago
|
||
Hi, I would like to work on this bug.
Comment 5•6 years ago
|
||
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
Comment 7•6 years ago
|
||
mack7121, do you need any help? Are you working on this?
Flags: needinfo?(mack7121)
Comment 8•6 years ago
|
||
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
Updated•6 years ago
|
Assignee: mack7121 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mack7121)
Comment 9•6 years ago
|
||
Hi :) Is this bug part of the Outreachy internship?
Comment 10•6 years ago
|
||
(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.
Comment 11•6 years ago
|
||
I would like to work on this.
Comment 12•6 years ago
|
||
Excellent. I have assigned this bug to you, blackbess. Please let me know if you have any questions.
Assignee: nobody → reddyrushi20
Status: NEW → ASSIGNED
Comment 13•6 years ago
|
||
:blackbess are you still working on this bug? Do you need some help?
Flags: needinfo?(reddyrushi20)
Comment 14•6 years ago
|
||
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)
Assignee | ||
Comment 15•6 years ago
|
||
Hi, Chris Could I work on this issue if this is still open? :)
Flags: needinfo?(chutten)
Comment 16•6 years ago
|
||
Certainly! Let me know if you have any questions.
Assignee: nobody → is2ei.horie
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)
Assignee | ||
Comment 17•6 years ago
|
||
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)
Comment 18•6 years ago
|
||
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)
Assignee | ||
Comment 19•6 years ago
|
||
Thanks Chris, I can run "./mach test toolkit/components/telemetry/tests/" now! I will continue to work on the issue.
Assignee | ||
Comment 20•6 years ago
|
||
Hi Chris, I attached a patch. Could you please check it? Thanks.
Attachment #8971851 -
Flags: review?(chutten)
Comment 21•6 years ago
|
||
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+
Assignee | ||
Comment 22•6 years ago
|
||
Thanks Chris! I added a test. Could you please review it?
Attachment #8971851 -
Attachment is obsolete: true
Attachment #8972297 -
Flags: review?(chutten)
Comment 23•6 years ago
|
||
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)
Assignee | ||
Comment 24•6 years ago
|
||
Thanks! I changed it not to use sys.maxint . Could you check it, please?
Attachment #8972473 -
Flags: review?(chutten)
Assignee | ||
Comment 25•6 years ago
|
||
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)
Assignee | ||
Comment 26•6 years ago
|
||
Ah, I'm sorry those my comments are duplicated.
Comment 27•6 years ago
|
||
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+
Comment 28•6 years ago
|
||
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)
Assignee | ||
Comment 29•6 years ago
|
||
Ok. Thanks!
Updated•6 years ago
|
Flags: needinfo?(chutten)
Keywords: checkin-needed
Comment 30•6 years ago
|
||
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
Comment 31•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9296d2a3287
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
status-firefox60:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•