Opened 2 years ago

Closed 16 months ago

#24296 closed enhancement (fixed)

Make Onionoo's document classes available as part of metrics-lib

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

Description

This ticket is based on a discussion on #24036:

  • At some point in the future we may want to move parts of org.torproject.onionoo.docs to metrics-lib, so that metrics-bot and other clients can re-use that code. More specifically, we should only move the externally provided JSON objects, not the internally used status objects.

Child Tickets

Change History (13)

comment:1 Changed 17 months ago by karsten

I made a start here in preparation of resolving #24295 afterwards.

But before I write more code, is there code in metrics-bot that I should be looking at first? Something that is worth reusing? Any insights from writing that code that we should keep in mind when writing this new code?

Thanks!

comment:2 Changed 17 months ago by irl

Possibly org.torproject.metrics.bot.tor.ZonedDateTimeDeserializer (although this was written for Gson, not Jackson). This is useful to provide a consistent date/time parsing to avoid having to parse it in every consumer of the documents.

I would like to have org.torproject.metrics.bot.tor.RelayFlag in metrics-lib, with a function to return these from details documents. I didn't write a deserializer for these yet though and it would also mean that the flags images would need to be included in metrics-lib.

There are also the org.torproject.metrics.bot.tor.{BaseRelay,Bridge,Relay} interfaces that could be included in metrics-lib. The generateBadge() methods can be ignored, I'll move these to seperate badge classes.

comment:3 in reply to:  2 ; Changed 17 months ago by karsten

Replying to irl:

Possibly org.torproject.metrics.bot.tor.ZonedDateTimeDeserializer (although this was written for Gson, not Jackson). This is useful to provide a consistent date/time parsing to avoid having to parse it in every consumer of the documents.

Okay, I'm not sure how to do this in Jackson, but I'll look into using ZonedDateTime rather than String for timestamps.

I would like to have org.torproject.metrics.bot.tor.RelayFlag in metrics-lib, with a function to return these from details documents. I didn't write a deserializer for these yet though and it would also mean that the flags images would need to be included in metrics-lib.

Hmm, the downside of having an enum like that is that we'll have to put out a new metrics-lib release whenever there's a new flag. I'll think more about this.

There are also the org.torproject.metrics.bot.tor.{BaseRelay,Bridge,Relay} interfaces that could be included in metrics-lib. The generateBadge() methods can be ignored, I'll move these to seperate badge classes.

Yes, I was planning to include interfaces like RelaySummaryDocument containing all fields in a relay details object, and the same for BridgeSummaryDocument, RelayDetailsDocument, etc. I'll take a look at those metrics-bot interfaces while writing those for metrics-lib.

Alright, I'll give this a try and let you know when I have something to look at. Thanks!

comment:4 in reply to:  3 ; Changed 17 months ago by irl

Replying to karsten:

Hmm, the downside of having an enum like that is that we'll have to put out a new metrics-lib release whenever there's a new flag. I'll think more about this.

Is this a lot of work?

comment:5 in reply to:  4 Changed 17 months ago by irl

Replying to irl:

Replying to karsten:

Hmm, the downside of having an enum like that is that we'll have to put out a new metrics-lib release whenever there's a new flag. I'll think more about this.

Is this a lot of work?

Aaaah, I see. It would need to know about the flag even before we want to do anything with it, because it would appear in the consensus. In this case it would not be a good idea to have this as an automatic serialization/deserialization.

If something else starts using the images then we can think about this again, but for now we should not include the enum.

comment:6 Changed 17 months ago by karsten

Status: newneeds_review

I made a start here, but I'm not sure whether I'll be able to continue working on this in the remainder of this week. Would you mind reviewing a first commit that prepares this change and that seems useful to do anyway? It's in commit e65bb30 in my task-24296 branch. Thanks!

Mote that I spent quite some time on the ZonedDateTime change mentioned above. Maybe we'll have to do that after moving things over to metrics-lib.

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

Replying to karsten:

Mote that I spent quite some time on the ZonedDateTime change mentioned above. Maybe we'll have to do that after moving things over to metrics-lib.

s/Mote/Note/

Sometimes it helps to sleep over something. I guess my mistake was that I wanted to change attribute types from String to ZonedDateTime, and that turned out to be more difficult with Jenkins than I had expected. What we could do, though, is leave attributes as String types and simply provide additional getters and setters that work with ZonedDateTime representations of those String values. I might try that out later today, though not now.

Leaving in needs_review for the first commit that is unrelated to dates.

comment:8 Changed 17 months ago by irl

Status: needs_reviewneeds_revision

The first commit looks ok, but I'd be happier with an explicit annotation on the class to indicate the naming strategy in case Jackson decides to change its default.

@JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy.class)
public class DetailsDocument extends Document {
  ...

When you say Jenkins, do you mean Jackson?

Additional getters and setters would be ok, but I think actually having the ZonedDateTime representations would be the more elegant solution.

Even though not the "recommended" method, this StackOverflow answer looks to contain an example of doing it in the way I had done it for Gson. This would avoid dependency on extra JARs which it looks like is the Jackson preferred way to do it.

comment:9 in reply to:  8 Changed 17 months ago by karsten

Replying to irl:

The first commit looks ok, but I'd be happier with an explicit annotation on the class to indicate the naming strategy in case Jackson decides to change its default.

@JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy.class)
public class DetailsDocument extends Document {
  ...

Agreed! In fact, we should consider adding explicit annotations for everything that we're currently configuring in the ObjectMapper. Ideally, these classes would be useful even without our class that configures an ObjectMapper.

When you say Jenkins, do you mean Jackson?

Absolutely!

Additional getters and setters would be ok, but I think actually having the ZonedDateTime representations would be the more elegant solution.

Even though not the "recommended" method, this StackOverflow answer looks to contain an example of doing it in the way I had done it for Gson. This would avoid dependency on extra JARs which it looks like is the Jackson preferred way to do it.

Agreed, this looks like an okay way of doing it.

Thanks for reviewing and commenting! I'll put this back on my list.

comment:10 Changed 17 months ago by karsten

Status: needs_revisionneeds_review

Oh, well. Even though the ZonedDateTime change doesn't look terribly hard, I'm still running into issues. I feel like this is a perfect-is-the-enemy-of-good case. How about we postpone the ZonedDateTime change and go with the Jackson-related changes only for now? In particular, would you like to review commit 403f107 in my task-24296 branch that adds annotations as discussed above? We can still make more changes later. Thanks!

comment:11 Changed 17 months ago by irl

Status: needs_reviewneeds_revision

Ok, we shouldn't spend longer on it than we have to.

Commit 403f107 looks good to me.

comment:12 Changed 17 months ago by karsten

Alright, cherry-picked e65bb30 and 403f107 and pushed them to master. Thanks for the reviews so far!

comment:13 Changed 16 months ago by karsten

Resolution: fixed
Status: needs_revisionclosed

The original purpose of this ticket, making Onionoo's classes available to other Onionoo clients, can now be accomplished by simply using Onionoo's thin jar as dependency. No need to move classes to metrics-lib anymore. I made a remark on #24870 regarding the ZonedDateTime discussion above. Other than that, there's nothing else to be done here. Closing. Thanks!

Note: See TracTickets for help on using tickets.