Opened 17 months ago

Closed 6 months ago

#23549 closed enhancement (implemented)

Move ExoneraTorServlet to metrics-web

Reported by: karsten Owned by: karsten
Priority: Medium Milestone:
Component: Metrics/Website Version:
Severity: Normal Keywords: metrics-2018
Cc: Actual Points:
Parent ID: Points:
Reviewer: irl Sponsor:

Description

As of today, ExoneraTor provides a query interface that can be used externally. We built that in order to split the service into the existing backend and a frontend that can live on Tor Metrics. Let's consider moving the page now, and that basically means moving ExoneraTorServlet to metrics-web.

Child Tickets

Attachments (1)

0001-Add-new-thin.jar-file-without-dependencies.patch (3.0 KB) - added by karsten 7 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 17 months ago by karsten

I have a (working) local branch that needs some more tweaking. But while working on it I ran into a question that we should probably find an answer for before starting the review-and-merge process:

Should we copy QueryResponse from ExoneraTor to metrics-web?

The thing is, we'll need to keep a copy in ExoneraTor, so that we can create and serialize a query response there. But we'll also need it in metrics-web to de-serialize and extract query response contents.

Hmmm. Reminds me of our index.json discussions. But I'm not clear whether we really want to do the same thing here and put it into an internal part of metrics-lib. Maybe. But maybe not. Thoughts?

comment:2 Changed 17 months ago by karsten

Status: newneeds_information

comment:3 Changed 17 months ago by iwakeh

I needed to think about this for a while. Moving the servlet is essentially reducing ExoneraTor to its backend functionality. Why not begin with releases of ExoneraTor now with the move of the servlet and using the released jar in metrics-web?

Steps involved for ExoneraTor:

  • create a war for replying to the query
  • create a jar for the internal (= unofficial&beta) API (QueryResponse alone)
  • move the servlet to metrics-web and use it there with the jar

Another, not required but highly simplifying process is using embedded jetty for the war, which should be easier without the servlet.

comment:4 Changed 17 months ago by iwakeh

Part of the above is the discussion started in #16597, which is simplyfied by removing the servlet.

comment:5 Changed 17 months ago by iwakeh

These two would also be achieved: #19624, #21145.

comment:6 Changed 17 months ago by karsten

Interesting. I'll have to think about this for a moment. :) Will get back here as soon as I have a better idea of how this could work.

comment:7 Changed 17 months ago by karsten

With an approach very similar to the one you describe above, do you think we could

  1. keep the current front-end functionality in ExoneraTor to enable others to run it stand-alone and
  2. generate a .jar file for metrics-web to use ExoneraTorServlet and QueryResponse there with different header and footer?

Note that I wouldn't want to keep the existing ExoneraTor front-end running on exonerator.tp.o. I'm just thinking whether we need to rip out this functionality or could instead leave ExoneraTor intact for those who don't happen to run a metrics-web, too. And now that I write this, I believe this use case is real, for people who can't use the public ExoneraTor, because they can't give out the IP addresses they're looking for.

All the other changes you mention, including using embedded Jetty, sound good to me, and I could see us making some of them in the course of making this move to metrics-web.

comment:8 Changed 17 months ago by karsten

Keywords: metrics-2018 added

comment:9 Changed 17 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:10 Changed 15 months ago by karsten

From #24292:

  • Reconsider moving some classes to metrics-lib for re-use from metrics-web, but not before the Jetty switch and possibly related changes are over. (Note: the Jetty switch and any related changes are already done.)

comment:11 Changed 14 months ago by iwakeh

Keywords: metrics-2018 added; metrics-2017 removed

Will be completed in 2018.

comment:12 Changed 7 months ago by karsten

Status: needs_informationneeds_review

I finally put some more thoughts into this ticket after spending some time on the related #24296.

I'm less convinced that moving classes from the ExoneraTor repository to the metrics-lib repository in order to use them in Tor Metrics (and possibly in a stand-alone version of ExoneraTor) is the best way to go.

Taking a step back, there are plenty of ways to reuse classes from the ExoneraTor repository in Tor Metrics:

  • Copy: We can just copy classes over. Well, okay, let's not.
  • Monorepo: This is an idea that crossed my mind the other week and that I still think is a good idea. However, let's not do that yet. Even though it would solve this problem.
  • Submodule: We could pull in ExoneraTor as Git submodule in Tor Metrics. Not my favorite.
  • Shared library: This is the metrics-lib plan discussed above. Would work, but we'd basically advance the technical interface between ExoneraTor backend and frontend to a public interface. I don't think we need to do this, and we should try to avoid the added effort for maintaining a public interface if we can. Note that we can always do that later, if we think it's a good idea.
  • Release as dependency: This was iwakeh's idea above, and I think I like that better than the shared library plan, for the reason stated above. However, exonerator-2.1.0.jar is 6.3M large, because it contains all of ExoneraTor's dependencies. It's a fat jar.
  • Thin jar as dependency: Based on iwakeh's idea (or maybe this was their idea all the time), provide another thin jar file together with ExoneraTor's next release and pull that into Tor Metrics. This jar would only contain ExoneraTor's own classes and resources but without all those other classes (Apache Commons, Jackson, Jetty, Logback, etc.). However, it would contain all of ExoneraTor's classes, not a subset. We could use the very same approach for Metrics Bot reusing Onionoo's document classes. But let's see first if it works for ExoneraTor.
  • What else did I miss?

Thoughts? Setting to needs_review for the concept, not for actual code.

comment:13 Changed 7 months ago by irl

Status: needs_reviewneeds_revision

For now I think the thin jar option is perhaps the best option.

I thought we had added a JSON output format for ExoneraTor. Can we not just use this from a servlet in metrics-web?

comment:14 in reply to:  13 ; Changed 7 months ago by karsten

Owner: changed from metrics-team to karsten
Status: needs_revisionassigned

Replying to irl:

For now I think the thin jar option is perhaps the best option.

Yes, that's also my preference.

I thought we had added a JSON output format for ExoneraTor. Can we not just use this from a servlet in metrics-web?

Yes, we would be using that JSON output format. However, we need (1) the exact same QueryResponse class for parsing the JSON output as for generating and (2) the same servlet for displaying results nicely as on https://exonerator.torproject.org/. (What I did not say yet is that I hope that we can use the same servlet in a standalone ExoneraTor as in an ExoneraTor page on Tor Metrics.)

Alright, looking more into the thin jar option. Thanks for commenting!

comment:15 in reply to:  14 Changed 7 months ago by karsten

Status: assignedneeds_review

Replying to karsten:

Alright, looking more into the thin jar option. Thanks for commenting!

Please review the metrics-base patch that I'm going to attach to this ticket in a second. It's going to create *-thin.jar files with the same contents as the *.jar file except for the dependencies that are not included here. I didn't manage to teach Ant not to produce such a thin jar file for metrics-lib, which by default already produces just a thin file, but I figured it might not even hurt to have both thin and fat jar files for metrics-lib. We'll just have to say so in the next metrics-lib change log.

comment:16 Changed 7 months ago by irl

Status: needs_reviewneeds_revision

This patch looks good to me.

Perhaps now there is a plan it is better to start again with a new ticket.

comment:17 in reply to:  16 Changed 7 months ago by karsten

Status: needs_revisionaccepted

Replying to irl:

This patch looks good to me.

Great! Pushed to metrics-base master.

Perhaps now there is a plan it is better to start again with a new ticket.

The comment history is indeed long. But I think I'd prefer to keep it by staying on this ticket. After all, it was useful to discuss the possible options. What we can do is write down what steps remain before resuming work here and maybe even edit the summary to point to the comment with the summary. And if that turns out to be still too confusing, we can then switch to a new ticket. Hope that works for you, too!

comment:18 Changed 7 months ago by karsten

Status: acceptedneeds_review

Alright, please review commit bd23511 in my exonerator task-23549 branch and commit a7ad91a in my metrics-web task-23549 branch.

Note on deployment:

  1. We'll first have to merge the ExoneraTor patch and put out a new ExoneraTor release, but without deploying yet.
  2. Then we'll have to update the metrics-web patch to include the released ExoneraTor thin jar, which we can deploy at the same time.
  3. Finally, we can deploy the ExoneraTor release, which will then redirect all requests (including parameters) to Tor Metrics.
  4. We should keep the redirect in place for a longer time than usual, because all earlier printouts of the ExoneraTor page still contain the permanent link to exonerator.tp.o, and we should make sure they still work for at least 6 months if not even 12 months or longer.

comment:19 Changed 6 months ago by irl

Reviewer: irl

Looking at this now.

comment:20 Changed 6 months ago by irl

Status: needs_reviewmerge_ready

The patch looks good, but like #27113 I definitely think this change would benefit from some release notes in addition to the changelog. In particular, we should point out where to change the servlet mapping in web.xml and possibly also give a private address for people to contact if we've broken their private Exonerator deployment (I guess there are some that wouldn't want to mail a mailing list).

comment:21 Changed 6 months ago by karsten

As discussed earlier today, let's call the next version 3.0.0, which should be a clear enough indication this is a backward-incompatible change. I'll open a new ticket for the release.

comment:22 Changed 6 months ago by karsten

Resolution: implemented
Status: merge_readyclosed

Cool! 3.0.0 is released, amended metrics-web patch is merged and deployed, 3.0.0 is deployed, change is announced. I guess this concludes this ticket/effort. Yay! Closing. Thanks!

Note: See TracTickets for help on using tickets.