Closed Bug 802914 Opened 12 years ago Closed 12 years ago

Implement Bagheera client

Categories

(Cloud Services :: Firefox: Common, defect)

Product:
Cloud Services
Component:
Firefox: Common
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla20
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file, 4 obsolete files)

Implement Bagheera client and server, v4
22.90 KB, patch
rnewman
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #718067 +++

FHR will need to talk to Bagheera for data submission. I'm a big fan of writing generic, standalone clients. In this bug I will create a standalone Bagheera JS client.
I'm proposing the creation of a new toolkit component called "metrics" which will hold all the code for Firefox Health Report and friends.

Dave Townsend (Toolkit module owner) should approve the creation of this component.

I /could/ put this code in toolkit/components/telemetry. But, I'd rather have multiple smaller, loosely coupled components than larger, tightly coupled ones. I find that produced code is more flexible and useful when things are done this way.
Attachment #672629 - Flags: review?(dtownsend+bugmail)
Placeholder for the implementation of a generic Bagheera JS client module. The code is incomplete, so I'm not asking for review yet. I'm just putting something up here so Dave will see that this component looks just like other things inside toolkit.

I'm also implementing a Bagheera HTTP server in JS so we can point unit tests (that run on buildbot) at it. We should ideally verify the behavior of the JS implementation against a trusted test suite maintained by the Metrics team. This is what we do in Sync land, where we have a test suite that can be used to verify any server implementation just be pointing the suite at any arbitrary URI. We don't need this as a prerequisite to land. But, it helps ensure that the tests are actually meaningful.
(In reply to Gregory Szorc [:gps] from comment #1)
> Created attachment 672629 [details] [diff] [review]
> Part 1: Skeleton for new "metrics" toolkit component, v1
> 
> I'm proposing the creation of a new toolkit component called "metrics" which
> will hold all the code for Firefox Health Report and friends.
> 
> Dave Townsend (Toolkit module owner) should approve the creation of this
> component.
> 
> I /could/ put this code in toolkit/components/telemetry. But, I'd rather
> have multiple smaller, loosely coupled components than larger, tightly
> coupled ones. I find that produced code is more flexible and useful when
> things are done this way.

I'm very much pro not confusing mdp/telemetry by sticking them in the same director. This stuff deserves own component. However, may I suggest /healthreport/ as a more descriptive name?
(In reply to Taras Glek (:taras) from comment #3)
> I'm very much pro not confusing mdp/telemetry by sticking them in the same
> director. This stuff deserves own component. However, may I suggest
> /healthreport/ as a more descriptive name?

+1
+  if (deleteOldID) {
+    request.setRequestHeader("X-Obsolete-Document", deleteOldID);
+  }

This is a privacy threat. This allows the server to track the client, by connecting the old and new ID. Please remove this.
(The threat still exists, if you delete this in a separate server call, because the server sees the 2 requests with the same IP address and can still connect them.)
Ben, as I'm sure you know, this particular aspect of the design has been discussed repeatedly, and at many levels of the project up to and including Brendan and Mitchell. The ability to measure user retention is a core requirement for this project and critically important for the long term success of Firefox. The reason for this is that our current lack of data about what type of users are using (or not using) Firefox is a major pain point for effectively prioritizing and addressing issues that impact our users.  The net effect of this is that we are unable to deliver all of the improvements users expect, and that hurts our ability to attract and retain users, which in turn hurts our influence over the direction of the Web.

To be clear, many of us share your concern about Mozilla taking a first step, however shallow and careful, towards tracking users more closely. It is not a decision anyone took lightly, or without due care and concern over the exact data that will be captured and how we will use it. Mozilla is an organization that cares deeply and passionately about user sovereignty, and we are very deliberate about every choice we make in terms of how and when to capture information about our users.  That we have pressed on with this project is a reflection of the our belief that this is a necessary decision in order to best serve our users and the Mozilla mission.

As of now, the concerns raised by yourself and others earlier this year were escalated to both Brendan and Mitchell, in their roles as the ultimate decision-makers for the project.  After extensive discussions and explorations, they have provided clear support to us in moving forward with the current plan, so we are moving forward.  Not everyone will agree with that decision, and we respect that, but in turn we ask that the community respect the governance process and the final decision that has been made.
(In reply to Gregory Szorc [:gps] from comment #1)
> Created attachment 672629 [details] [diff] [review]
> Part 1: Skeleton for new "metrics" toolkit component, v1
> 
> I'm proposing the creation of a new toolkit component called "metrics" which
> will hold all the code for Firefox Health Report and friends.

What are these friends? healthreport sounds better to me too.
Comment on attachment 672629 [details] [diff] [review]
Part 1: Skeleton for new "metrics" toolkit component, v1

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

::: toolkit/Makefile.in
@@ +16,5 @@
>    devtools \
>    forgetaboutsite \
>    identity \
>    locales \
> +  metrics \

This includes toolkit/metrics yet the other files are in toolkit/components/metrics...

I'd prefer it at toolkit/healthreport unless there is something else going in here that I am not aware of.
Attachment #672629 - Flags: review?(dtownsend+bugmail) → review-
(In reply to Dave Townsend (:Mossop) from comment #9)
> I'd prefer it at toolkit/healthreport unless there is something else going
> in here that I am not aware of.

There are two distinct phases to what :gps is working on:
1. a generic metric collection and submission framework
2. the set of metrics specifically needed for the Firefox Health Report feature.

My suggestion for consideration is that the generic part live in something like toolkit/metrics and the parts specific to FHR (that don't already belong somewhere) live in something like toolkit/healthreport
> The ability to measure user retention is a core requirement for this project and
> critically important for the long term success of Firefox.

I understand and respect that need. My alternative data collection proposal *does* allow to measure user retention and measure specific reasons why people stop using Firefox, but without tracking whole user profiles, but completely anonymous. I wasn't speaking about that, though.

I was just saying that the "delete" implementation is problematic. If you're saying that you critically need this "delete" operation for the analysis to work, you are admitting that you *will* use this information from the "delete" operation to connect the profiles and track users.

*** Please confirm/deny. ***

> Mozilla ... towards tracking users more closely

Oh-kay. FWIW, the whole reason why I use Mozilla is security/privacy and openness/freedom. Without that, I am better off with Chrome. If you're saying that tracking me is the direction you want to take, then I am gone. And most of Germany will leave with me, because most people in Europe (where Firefox is biggest) use Firefox for these very same reasons, because Firefox stands for openness/freedom and security/privacy.

> escalated to both Brendan and Mitchell

I haven't spoken a single word with them about this, and I was the one who argued the most against it.

For the record, I wasn't objecting to this project here, just to the above 3 lines of code. That's why I'd like the confirmation above.
Please do not discuss political issues on technical implementation bugs. Instead, please take it to dev-planning.

Besides, this bug is about implementing a generic JS module for communicating with a generic data storage service. There is nothing in this bug dealing with data collection.
> For the record, I wasn't objecting to this project here, just to the above 3 lines of code.
Depends on: 803377
(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #10)
> (In reply to Dave Townsend (:Mossop) from comment #9)
> > I'd prefer it at toolkit/healthreport unless there is something else going
> > in here that I am not aware of.
> 
> There are two distinct phases to what :gps is working on:
> 1. a generic metric collection and submission framework
> 2. the set of metrics specifically needed for the Firefox Health Report
> feature.
> 
> My suggestion for consideration is that the generic part live in something
> like toolkit/metrics and the parts specific to FHR (that don't already
> belong somewhere) live in something like toolkit/healthreport

While they are conceptually separate, I'm OK with them both living in the same component for now. Worst case we move some files around later. This may break a few import paths. But, that's a minor pain. The important bit is that I'll be writing modular code that is loosely coupled. It will be much easier to refactor and create division lines later.
(In reply to Gregory Szorc [:gps] from comment #0)
> I'm a big fan of
> writing generic, standalone clients. In this bug I will create a standalone
> Bagheera JS client.

If that is a goal here, then arguably shouldn't the client & server JSMs be in either /toolkit/modules/ or /toolkit/mozapps/bagheera ?
I have no clue how things work in Toolkit since I have never committed to it outside of some minor add-ons patches: you tell me.

I will be producing a bunch of generic JavaScript modules initially. Some may belong in existing components. But, I'm not sure about what modules exist or what belongs where. I'm relying on people to tell me "hey, this things seems like it should go in X."

If someone steps up and says "put X in Y," I probably won't object. But, if you pose it as a question, I'll just be like "I dunno, you tell me."

Perhaps if there were README files in toolkit/ I could figure this out for myself...
(In reply to Blair McBride (:Unfocused) from comment #15)
> If that is a goal here, then arguably shouldn't the client & server JSMs be
> in either /toolkit/modules/ or /toolkit/mozapps/bagheera ?

toolkit/mozapps is some weird historical artifact, I don't think we should expand its use to anything new. I'd like a toolkit/modules for something like bug 803377, but a component like this one seems a bit big for that.
(In reply to Gregory Szorc [:gps] from comment #12)
> Besides, this bug is about implementing a generic JS module for
> communicating with a generic data storage service. There is nothing in this
> bug dealing with data collection.

/Especially/ if this is for arbitrary "communicating with a generic data storage service" use cases, it seems to me that Ben is right that there's technically no need to send two IDs in one request in order to delete an old record. A separate delete request seems like a reasonably cheap way to enable the same API, assuming this is really just about getting the old record deleted.
Comment on attachment 672631 [details] [diff] [review]
Part 2: Implement Bagheera client and server, v1

>+++ b/toolkit/components/metrics/bagheeraclient.jsm

>+ * Information about Bagheera is available at

>+const EXPORTED_SYMBOLS = ["BagheeraClient"];

Bagheera is a great code name, but please don't use it anywhere in the code.
(In reply to Dão Gottwald [:dao] from comment #18)
> A separate delete request seems like a reasonably cheap way to
> enable the same API, assuming this is really just about getting the old
> record deleted.

See comment 6.
(In reply to Ben Bucksch (:BenB) from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #18)
> > A separate delete request seems like a reasonably cheap way to
> > enable the same API, assuming this is really just about getting the old
> > record deleted.
> 
> See comment 6.

Tracking based on the IP is a lot muddier (lots of users sharing IPs, for instance) and likely unattractive for non-malicious servers interested in accurate data on users, especially if they can get that data without connecting requests.
(In reply to Dão Gottwald [:dao] from comment #19)
> Bagheera is a great code name, but please don't use it anywhere in the code.

Bagheera is the name of the server/service: https://github.com/mozilla-metrics/bagheera. I am writing a client for that service. Therefore, I have a Bagheera client.

What other name would you choose? I can't say "Metrics Server Client" or the like because someone could stand up another instance of Bagheera that has nothing to do with "metrics!" The client is generic and would still talk to it. So, "Bagheera Client" is still more appropriate.

If Bagheera were an implementation of some other specification, I would gladly change the client's name to reflect the name of the underlying thing. But AFAICT there is no such specification and Bagheera is the most generic name we have.

Until someone tells me a better name that makes more sense than Bagheera, the name stays.
(In reply to Ben Bucksch (:BenB) from comment #11)
> I was just saying that the "delete" implementation is problematic. If you're
> saying that you critically need this "delete" operation for the analysis to
> work, you are admitting that you *will* use this information from the
> "delete" operation to connect the profiles and track users.
> 
> *** Please confirm/deny. ***

That is not correct at all.  We have no intent or desire around connecting documents or tracking user profiles over time, and we will not retain any data from the deleted documents.

The reason for deleting the previous document is far less nefarious than you make it out to be: we want to remove data that will negatively impact our analysis.  If a document still exists after some time, it can be safely assumed that the instance that uploaded that document is no longer active.  This is what will allow us to accurately and reliably perform analysis on the set of installs which go inactive.  Without deleting obsolete documents, we have no deterministic way to separate documents between inactive and active users, making the data we are collecting unusable for our core purpose.
(In reply to Gregory Szorc [:gps] from comment #22)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > Bagheera is a great code name, but please don't use it anywhere in the code.
> 
> Bagheera is the name of the server/service:
> https://github.com/mozilla-metrics/bagheera. I am writing a client for that
> service. Therefore, I have a Bagheera client.
> 
> What other name would you choose? I can't say "Metrics Server Client" or the
> like because someone could stand up another instance of Bagheera that has
> nothing to do with "metrics!" The client is generic and would still talk to
> it. So, "Bagheera Client" is still more appropriate.

If you describe what the module is meant to do (which you should if you want to land it as a general-purpose module), I can try to come up with a name. "Client to talk to the Bagheera server" is kind of meaningless, as it's unclear what a Bagheera server is in general terms; it's neither self-explaining nor documented anywhere. The readme on github has the Metrics reference baked in that you wanted to avoid ("REST service for Mozilla Metrics"), which makes sense since AFAIK the server module was open-sourced mostly as an act of transparency rather than for direct re-use in other contexts.
(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #10)
> (In reply to Dave Townsend (:Mossop) from comment #9)
> > I'd prefer it at toolkit/healthreport unless there is something else going
> > in here that I am not aware of.
> 
> There are two distinct phases to what :gps is working on:
> 1. a generic metric collection and submission framework
> 2. the set of metrics specifically needed for the Firefox Health Report
> feature.
> 
> My suggestion for consideration is that the generic part live in something
> like toolkit/metrics and the parts specific to FHR (that don't already
> belong somewhere) live in something like toolkit/healthreport

Sounds fine to me. Will the healthreport stuff vary from Firefox to Fennec etc?
(In reply to Blair McBride (:Unfocused) from comment #15)
> (In reply to Gregory Szorc [:gps] from comment #0)
> > I'm a big fan of
> > writing generic, standalone clients. In this bug I will create a standalone
> > Bagheera JS client.
> 
> If that is a goal here, then arguably shouldn't the client & server JSMs be
> in either /toolkit/modules/ or /toolkit/mozapps/bagheera ?

Unless we have concrete plans to use this client for other purposes right now it should just go in with the rest of the metrics stuff in toolkit/metrics
(In reply to Gregory Szorc [:gps] from comment #22)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > Bagheera is a great code name, but please don't use it anywhere in the code.
> 
> Bagheera is the name of the server/service:
> https://github.com/mozilla-metrics/bagheera. I am writing a client for that
> service. Therefore, I have a Bagheera client.

If that's the name of the protocol it is implementing then that name seems fine to me.
(In reply to Dave Townsend (:Mossop) from comment #25)
> Sounds fine to me. Will the healthreport stuff vary from Firefox to Fennec
> etc?

There is likely to be significant overlap from desktop to mobile because they have a lot of the same functionality.  It is expected that there would be some customizations for mobile though.

If you look at a farther out case like Firefox OS, there would be significantly more divergence because of the difference between "just a browser" and an OS.
(In reply to Dave Townsend (:Mossop) from comment #27)
> (In reply to Gregory Szorc [:gps] from comment #22)
> > (In reply to Dão Gottwald [:dao] from comment #19)
> > > Bagheera is a great code name, but please don't use it anywhere in the code.
> > 
> > Bagheera is the name of the server/service:
> > https://github.com/mozilla-metrics/bagheera. I am writing a client for that
> > service. Therefore, I have a Bagheera client.
> 
> If that's the name of the protocol it is implementing then that name seems
> fine to me.

It's just a server module, there's no formalized protocol, as I understand it. Anyway, if this lands in toolkit/metrics/, then "Bagheera client" seems ok to me as well, fwiw.
Comment on attachment 672629 [details] [diff] [review]
Part 1: Skeleton for new "metrics" toolkit component, v1

Patch moved to bug 718067.
Attachment #672629 - Attachment is obsolete: true
Moving to Services product because it was decided in bug 718067 that this is not landing in Toolkit.

rnewman: Now that we're putting code in services/, I was thinking it makes sense to put this in services/common. We already have our generic clients/servers there. Look for a new patch soon...
Component: General → Firefox: Common
Product: Toolkit → Mozilla Services
No longer blocks: 718067
Refreshed patch. Haven't given this a good one-over yet. There are likely parts that are horribly wrong. Please tell me what they are.
Attachment #672631 - Attachment is obsolete: true
Attachment #677913 - Flags: feedback?(xstevens)
Attachment #677913 - Flags: feedback?(rnewman)
Blocks: 808109
No longer blocks: 718066
Blocks: 808219
No longer blocks: 808109
Comment on attachment 677913 [details] [diff] [review]
Implement Bagheera client and server, v2

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

::: services/common/bagheeraclient.js
@@ +108,5 @@
> +      throw new Error("payload argument must be defined.");
> +    }
> +
> +    // TODO investigate Bagheera HTTP spec and see whether URI escaping is
> +    // necessary or whether GIGO applies.

That doesn't make sense to me. Surely you're taking two strings as input and trying to compose a URI, so you always need to escape the components…?

I can't imagine that you'd want to allow id="../../foo" unescaped…

@@ +159,5 @@
> +
> +      if (response.status == 201) {
> +        result.serverSuccess = true;
> +      } else {
> +        result.serverSuccess = false;

Attach the response status etc. to the result?

::: services/common/modules-testing/bagheeraserver.js
@@ +35,5 @@
> +  /**
> +   * Whether this server has a namespace defined.
> +   *
> +   * @param ns
> +   *        (string) Namepsace whose existence to query for.

for whose existence to query.

@@ +46,5 @@
> +  /**
> +   * Whether this server has an ID in a particular namespace.
> +   *
> +   * @param ns
> +   *        (string) Namespace to look for item in.

in which to look for an item with the provided ID.

@@ +119,5 @@
> +  /**
> +   * Obtain the URI of a document in this server.
> +   */
> +  documentURI: function documentURI(ns, id) {
> +    return this.serverURI + ":" + this.port + "/1.0/submit/" + ns + "/" + id;

Can we lift out "/1.0/submit/"? It's used everywhere.
Attachment #677913 - Flags: feedback?(rnewman) → feedback+
(In reply to Gregory Szorc [:gps] from comment #31)
> Moving to Services product because it was decided in bug 718067 that this is
> not landing in Toolkit.
> 
> rnewman: Now that we're putting code in services/, I was thinking it makes
> sense to put this in services/common. We already have our generic
> clients/servers there. Look for a new patch soon...

Since services/common/ is your unofficial pseudo-toolkit as you said in a different bug, the above comment about naming, reusability etc. still apply.
(In reply to Dão Gottwald [:dao] from comment #34)

> Since services/common/ is your unofficial pseudo-toolkit as you said in a
> different bug, the above comment about naming, reusability etc. still apply.

If you mean:

---
It's just a server module, there's no formalized protocol, as I understand it. Anyway, if this lands in toolkit/metrics/, then "Bagheera client" seems ok to me as well, fwiw.
---

and

---
Unless we have concrete plans to use this client for other purposes right now it should just go in with the rest of the metrics stuff in toolkit/metrics
---

then:

(a) Great; glad we're agreed that Bagheera is the least-bad name we can pick right now.

(b) Our protocol implementations live in common/ -- bagheera will be alongside aitc and storageserver. This enforces testability (including cross-testing of the two implementations against each other's test suites) and loose coupling, both of which are desirable even if the protocol client is never reused.


Could you clarify/reiterate if you have other opinions that you'd like us to bear in mind?
This should live near metrics code since it's only relevant to metrics. Tests can still work like you describe.
(In reply to Dão Gottwald [:dao] from comment #36)
> This should live near metrics code since it's only relevant to metrics.

OK, thanks for clarifying.

I think it's less relevant to metrics than the AITC and Sync storage protocol clients are to AITC and Sync 2.0. Both of those are services/common modules, and I have a fairly strong preference to preserve symmetry here.

I don't see any technical advantage in putting the Bagheera client in services/common/metrics or services/metrics, and it would mildly inhibit possible future reuse through bad naming.

I think services/common is pretty near to services/metrics, so I'm happy with Greg's proposed layout.

Thanks for weighing in!
(In reply to Richard Newman [:rnewman] from comment #37)
> (In reply to Dão Gottwald [:dao] from comment #36)
> > This should live near metrics code since it's only relevant to metrics.
> 
> OK, thanks for clarifying.
> 
> I think it's less relevant to metrics

Really? Bagheera == "REST service for Mozilla Metrics", as far as I can tell.

> than the AITC and Sync storage
> protocol clients are to AITC and Sync 2.0. Both of those are services/common
> modules, and I have a fairly strong preference to preserve symmetry here.

Presumably AITC and Sync storage could be moved to sync-specific locations.

> I don't see any technical advantage in putting the Bagheera client in
> services/common/metrics or services/metrics,

It would make this code more self-contained and approachable for newcomers.

> and it would mildly inhibit possible future reuse through bad naming.

I suspect that's pie in the sky, but moving from a feature-specific location to a general one is always an option.

> I think services/common is pretty near to services/metrics, so I'm happy
> with Greg's proposed layout.

According to Gregory, services/common/ is as generic as toolkit/ (the former just isn't reviewed by toolkit peers). This means that it's not at all near.

> Thanks for weighing in!

You're welcome.
(In reply to Dão Gottwald [:dao] from comment #38)

> Really? Bagheera == "REST service for Mozilla Metrics", as far as I can tell.

This code is for a very generic REST document storage API. My point was that it's way less coupled to a metrics product than the Sync storage protocol is to Sync. You could write a blog client and server using Bagheera with few or no changes (none at all on the client), and we might well end up doing something along these lines -- e.g., submitting Sync error logs with this protocol, which we've wanted to do for years.

That's why I oppose characterizing it as 'metrics' -- that would be like saying HTTP is for transferring HTML.


> Presumably AITC and Sync storage could be moved to sync-specific locations.

I still haven't heard a reason why we would want to do that.

AITC and Sync are two different services integration products. That's why they both live in services/, with the reusable protocol code in s/common, and the app code in s/$app.

services/ is where we put code to integrate with hosted services. s/common is where we land non-app and reusable code, at least until it warrants movement to encourage use outside of s-i.


> According to Gregory, services/common/ is as generic as toolkit/ (the former
> just isn't reviewed by toolkit peers). This means that it's not at all near.

Greg said three things, which don't quite mean that.

* Nothing in services/common is Sync-specific
* services/common is essentially our team's toolkit
* We would like to uplift some things, including log4moz (Bug 451283), into toolkit to encourage wider use.
Newman [:rnewman] from comment #39)
> (In reply to Dão Gottwald [:dao] from comment #38)
> 
> > Really? Bagheera == "REST service for Mozilla Metrics", as far as I can tell.
> 
> This code is for a very generic REST document storage API. [...]

Then we're back to the question of proper naming and documentation. "Bagheera" refers to a custom and under-documented Metrics server module. "RestDocumentStorage" might be a good start for a name, based on your description.

> > Presumably AITC and Sync storage could be moved to sync-specific locations.
> 
> I still haven't heard a reason why we would want to do that.

Take necko as a model: protocol code is in netwerk/protocol/. "netwerk" would be comparable to "metrics" or "sync" rather than "services"; the latter is just a pretty much arbitrary effect of team structures. Putting code in services/common/ that isn't common to multiple services makes things increasingly messy as more services get added (i.e. now).

> * Nothing in services/common is Sync-specific

But this is purely hypothetical for some pieces, right?
Please engineer for actual usecases, not for some theoretical possibilities that might be possible with the spec. "services" is way too generic.
(In reply to Dão Gottwald [:dao] from comment #40)

> Then we're back to the question of proper naming and documentation.
> "Bagheera" refers to a custom and under-documented Metrics server module.
> "RestDocumentStorage" might be a good start for a name, based on your
> description.

That Bagheera poorly documented and not widely used doesn't mean that this isn't a client for it… it's a client for Bagheera (which is a generic REST document storage API), not for _all_ REST document storage APIs.

Can we take improving docs, writing up a standalone spec, and working on deployability as an action for xstevens and co, and move on?

> Putting code in services/common/ that isn't common to multiple services makes
> things increasingly messy as more services get added (i.e. now).

Fair enough.

I'd be happy to introduce services/protocols/ for the various protocols and test servers that currently live under common/, leaving that directory as "s-i's toolkit". I'd regard that as an improvement!

Would that work for you?
If you think I need to flush out more detail for you in the documentation please let me know, I'm happy to fix it.
Hooked up make target to run a standalone Bagheera server. Other misc changes.
Attachment #677913 - Attachment is obsolete: true
Attachment #677913 - Flags: feedback?(xstevens)
Attachment #679266 - Flags: review?(rnewman)
Attachment #679266 - Flags: feedback?(xstevens)
Forgot to add a new file.
Attachment #679266 - Attachment is obsolete: true
Attachment #679266 - Flags: review?(rnewman)
Attachment #679266 - Flags: feedback?(xstevens)
Attachment #679280 - Flags: review?(rnewman)
Attachment #679280 - Flags: feedback?(xstevens)
Comment on attachment 679280 [details] [diff] [review]
Implement Bagheera client and server, v4

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

I'm pretty happy with this. It doesn't have 100% test coverage, but if we waited to get 100% test coverage, we would never be able to ship features on such an abbreviated schedule.

If consensus is that we create services/protocols, we can do that in a follow-up before we merge.

::: services/common/bagheeraclient.js
@@ +70,5 @@
> +  _loadFlags: Ci.nsIRequest.LOAD_BYPASS_CACHE |
> +              Ci.nsIRequest.INHIBIT_CACHING |
> +              Ci.nsIRequest.LOAD_ANONYMOUS,
> +
> +  _timeout: 5 * 60 * 1000, // 5 minutes

DEFAULT_TIMEOUT_MSEC

@@ +72,5 @@
> +              Ci.nsIRequest.LOAD_ANONYMOUS,
> +
> +  _timeout: 5 * 60 * 1000, // 5 minutes
> +
> +  _RE_URI_IDENTIFIER: new RegExp("^[a-zA-Z0-9_-]+$"),

Use a /regex literal/.

@@ +185,5 @@
> +    }
> +
> +    if (!this._RE_URI_IDENTIFIER.test(id)) {
> +      throw new Error("Illegal id value. Must be alphanumeric + [_-]: " + id);
> +    }

That works for me.

@@ +215,5 @@
> +      default:
> +        result.serverSuccess = false;
> +
> +        this._log.info("Received unexpected status code: " + response.status);
> +        this._log.info("Response body: " + response.body);

`info` is probably the wrong level here. It's either `warn` or `debug`…

I still think that returning a failure without including some machine-processable information is wrong: attach the response status (and perhaps body) to the result.

::: services/common/tests/unit/test_bagheera_client.js
@@ +6,5 @@
> +Cu.import("resource://services-common/bagheeraclient.js");
> +Cu.import("resource://services-common/rest.js");
> +Cu.import("resource://testing-common/services-common/bagheeraserver.js");
> +
> +function getClientAndServer(port=8080) {

Lift 8080 into a const.
Attachment #679280 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #46)
> 
> @@ +215,5 @@
> > +      default:
> > +        result.serverSuccess = false;
> > +
> > +        this._log.info("Received unexpected status code: " + response.status);
> > +        this._log.info("Response body: " + response.body);
> 
> `info` is probably the wrong level here. It's either `warn` or `debug`…
> 
> I still think that returning a failure without including some
> machine-processable information is wrong: attach the response status (and
> perhaps body) to the result.

The original RESTRequest (which has the response in a property) is always attached.
https://hg.mozilla.org/projects/larch/rev/7e8f51441fe1

With regard to creating services/protocols: should we do it? Possibly. As a prerequisite to merge? No. Why? This code will need to support B2G and backporting the refactor to Aurora is unnecessary work. We can paint the bike shed in the next release cycle.
Whiteboard: [fixed-in-larch]
Comment on attachment 679280 [details] [diff] [review]
Implement Bagheera client and server, v4

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).
Attachment #679280 - Flags: approval-mozilla-beta?
Attachment #679280 - Flags: approval-mozilla-aurora?
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/c73d02e3cc39
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 679280 [details] [diff] [review]
Implement Bagheera client and server, v4

Approving as this work blocks B2G ADI pings, and is pref'd off & disabled in Desktop/Mobile.
Attachment #679280 - Flags: approval-mozilla-beta?
Attachment #679280 - Flags: approval-mozilla-beta+
Attachment #679280 - Flags: approval-mozilla-aurora?
Attachment #679280 - Flags: approval-mozilla-aurora+
Depends on: 815320
Attachment #679280 - Flags: feedback?(xstevens)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: