Architectural Decision Records

[t2gran]

We have talked about adding Architectural Decision Records as a tool to keep track of important decisions taken in developer meetings about design and architecture. This PR contain:

• Process documentation and requeierments
• A initial ADR log
• Template for ADR issues
• Updated documentation
• ADR is excluded from the Stalebot

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.94%. Comparing base (ceb1613) to head (254657f).
Report is 3428 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5360      +/-   ##
=============================================
+ Coverage      66.44%   68.94%   +2.49%     
- Complexity     15211    19942    +4731     
=============================================
  Files           1786     1970     +184     
  Lines          69236    86477   +17241     
  Branches        7311     9828    +2517     
=============================================
+ Hits           46005    59618   +13613     
- Misses         20751    23949    +3198     
- Partials        2480     2910     +430     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jtorin]

Isn't ADR-7 and 8 associated to ADR-2?

[abyrd]

I do think 7 and 8 could be merged into 2. Also, 8 does not specify whether we are talking about JavaDoc and could be interpreted as manual documentation in some other file.

[abyrd]

This seems like a good idea to me. I added some comments suggesting small changes, but my main concern is the distinction between code style and architectural decisions, and avoiding redundancy and contradictions between them. I plan to have a short discussion with @t2gran about this PR before adding a final review.

[abyrd]

I do think 7 and 8 could be merged into 2. Also, 8 does not specify whether we are talking about JavaDoc and could be interpreted as manual documentation in some other file.

[abyrd]

The word "clean" here could have many interpretations, and even conflicting interpretations by different people. It may be effectively equivalent to "keep code good". A reader might not know if the following two points are elucidating the meaning of "clean" or if they are in addition to cleanness.

[abyrd]

Many of these Architectural Decisions could be seen as code conventions and overlap with https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/CODE_CONVENTIONS.md. For example, use of records is specifically discussed in the code conventions document. Do we have guidelines on what is a code convention and what is an architectural decision? This could be important for both writers and readers to avoid redundancy and inconsistency.

[t2gran]

I suggest we break code conventions into smaller pieces and list them in "ADRs", we could also try to break the ADR list up into more than one list (using separate prefixes).

[abyrd]

This design decision just states the name of a problem. It should be rephrased to state that this problem should be avoided and how to do so. The linked document contains detailed suggestions on how to avoid or eliminate the problem, but the ADR item does not mention this.

Also, as mentioned on a comment below, the distinction between code style and architectural decisions does not seem clear.

[t2gran]

I do not think we should repeat something that is better documented elsewhere, but we should express it as a sentence: "Respect/Follow/Avoid Feature envy".

[abyrd]

I discussed this at length today with @t2gran. To summarize, this seems like a good idea, but we will need to follow up with some other changes to update and reorganize documentation. We may want to make some alterations to the ADR concept and clearly explain our interpretation in the introductory paragraphs of the newly created documents. Some of the points below can be taken care of in a final revision of this PR, and others in follow-up changes.

In the materials at https://adr.github.io/ and https://github.com/joelparkerhenderson/architecture-decision-record make it clear that their intent is to standardize some vocabulary and the form of the decision records so they are easy for people to understand across projects.

There is a strong emphasis throughout these materials on justifying the decisions and placing them in context, while keeping the records very short and compact. At https://github.com/joelparkerhenderson/architecture-decision-record#what-is-an-architecture-decision-record we see the definition "An architecture decision log (ADL) is the collection of all ADRs created and maintained for a particular project (or organization)." So this PR is actually creating an ADL composed of ADRs - file names and PR details should be updated accordingly.

We may want to adopt and follow the more specific MADR conventions (https://adr.github.io/madr/) because we plan to use markdown, we will be recording Any decision rather than Architectural decisions (see below). In that case we can refer to MADR and MADL (record and log).

@t2gran wants this decision log to serve as a checklist when evaluating pull requests, allowing reviewers to quickly look through the list as a reminder. To this end he wants to eventually factor out the context and justification discussion into separate markdown files, one per decision, in the same directory with the decision log. Only a very brief title would appear in the log, and link to the detailed file. This title should not just be a subject but a very short rule (e.g. not "DRY" but "Factor out repeated code to improve maintainability (DRY)").

@abyrd points out that ADRs seem to conventionally have unique identifiers that are increasing integers, assigned in order of decisions. This establishes a chronological list as the canonical ordering and presentation of the decisions, requiring manual maintenance of additional index files that group the decisions by type and topic (e.g. code convention vs. architecture, and placing different identifier naming convention points adjacent to each other in the code convention section). Any revision or reordering of these points as part of a documentation improvement would require significant maintenance effort (keeping links live and up to date, ensuring consistency and completeness of different pages that purport to be full lists or indexes of decision records).

There appear to be content management systems that would automatically manage and index the decision records, but neither of us thinks it's a good idea to add more tools to manage the ADL. Therefore it may be a good idea to give each decision a short compact but non-sequential identifier. In PR reviews and discussions the decisions can be referenced by these short names or short statements of their content (i.e. "according to project decision 'DRY principle'..." instead of "according to ADR-5..."). There then needs to be only one full canonical list, which can be ordered and reordered by topic with minimal maintenance effort. It turns out that use of non-sequential identifiers is prescribed and explained at: https://adr.github.io/madr/decisions/0006-use-names-as-identifier.html and MADR also provides an example of the first decision record being one for the use of MADR itself.

The core point in @abyrd's initial review of this PR was that the ADRs seem to overlap with code style or code conventions, and it's not clear whether ADRs are constrained to only architecture decisions (that is, abstractions or structural descriptions of a complex system that make the big picture understandable). We both agree it is useful to document decisions that impose rules on development, whether those are architectural, stylistic, or process decisions. It appears that other people have already run into this question, and MADR was broadened to "any" decisions rather than "architectural" decisions: "There are debates about what is an architecturally-significant decision and which decisions are not architecturally significant. Since we believe that any (important) decision should be captured in a structured way, we offer the MADR template to capture any decision. This repository offers a solution to record any decisions. It provides files to document any decisions using Markdown and Any Decision Records." (https://adr.github.io/madr/#overview)

[abyrd]

Continuing the summary of today's discussion:

General Documentation Notes

We've got some overlapping, redundant, and poorly named documentation that we should reorganize later after this PR. The ADL will overlap with the following documentation:

• https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/docs/Codestyle.md
• https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/docs/Developers-Guide.md
• https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/CODE_CONVENTIONS.md
• https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/ARCHITECTURE.md

Some ideas for categorizing and restructuring this kind of documentation:
Our documentation has (or could have) several different audiences:

• software developers (backend)
• software developers (UI)
• deployment users (product owners or POs)
• end users

Each one of these audiences might want their own version of:

• Getting started guide
• Rules / best practices to follow
• Support and community (lists, chats, meetings)

Each section should state clearly who the audience is, what exactly the documentation is about, and provide links to similar documentation for other audiences. This should make the exact titles less critical (although we still want to carefully choose good titles).

Note that some of this documentation is markdown files inside the source tree (easily accessible and up to date when coding on development branches) and other documentation is published on the public web once per release. The public documentation currently includes some development documentation that should be moved together with the other in-tree markdown. This includes some developer "getting started" material and the localization section, which is largely about UI code that is now treated as a developer debug interface (so these docs should be moved into the source tree with the other dev documentation). The release checklist can also be moved into the source tree, not the published documentation.

We discussed the possibility of publishing all documentation, including internal development documentation, publicly on the web, but even though this could be done automatically it could lead to some confusion. Github makes it straightforward to browse and read properly rendered markdown directly within the source tree. All software developers will likely be comfortable with looking at files directly on Github, and this ensures they're seeing current up-to-date files rather than obsolete ones from the last release. So we'll move toward having almost all developer documentation as markdown files in source tree folders (outside /doc) and only a short well-named development intro in the published documentation.

We may want to create a new issue to summarize any planned changes to the documentation based on the above, once this PR is merged and it's clear what remains to be updated in the documentation.

ADR

Much of our code will not be consistent with the documented architectural decisions because these rules have changed or become more clear over time. We do not have the resources to update everything to match new principles and rules all at once. Rather, old code should be brought up to new rules as we work on it. We should explain that this is the case at the beginning of the ADR document. We should also mention there that it's not purely architectural decisions. Decision records can probably be categorized into architecture, style (code conventions), and process.

Example of factoring details of records out into separate MD files:
https://docs.devland.is/technical-overview/adr




[abyrd]

I will push an ADR in response to this background discussed at the last developer meeting:
Long ago, OpenTripPlanner originally used Spring dependency injection, with components wired up via
an XML application context. Many developers found this problematic due to "programming in markup"
and the difficulty in tracing and debugging. There was a backlash against dependency injection,
Spring was completely removed from the codebase, and OTP passed information around in context
objects with no framework for this purpose. As of July 25, 2022 in PR #4289 OTP switched over to
using the Dagger library. The fact that Dagger is configured and operates via imperative Java code
relieved most of the concerns about other dependency injection approaches and past experiences.

[2martens]

I will push an ADR in response to this background discussed at the last developer meeting: Long ago, OpenTripPlanner originally used Spring dependency injection, with components wired up via an XML application context.

Very illuminating. We use Spring Boot in our application and there is no XML configuration for dependency injection. Classes annotated with @Component or similar annotations are automatically available to be injected.

[abyrd]

I will push an ADR in response to this background discussed at the last developer meeting: Long ago, OpenTripPlanner originally used Spring dependency injection, with components wired up via an XML application context.

Very illuminating. We use Spring Boot in our application and there is no XML configuration for dependency injection. Classes annotated with @Component or similar annotations are automatically available to be injected.

We are talking about 10+ years ago, but there was such a thing as "autowiring" at that time too. I believe we first transitioned from explicit XML wiring to using a lot of autowiring, but there was still a general negative experience of objects being instantiated and references injected automatically. Even if defined declaratively and managed automatically, at runtime actions still do happen in some order, and while it's convenient to delegate that reasoning to a library or tool in simple cases, when you encounter more complex cases you ended up having to reverse-engineer the tool's reasoning and trick it into doing what you want. This can end up being more opaque and harder to maintain than just explicitly describing the imperative steps you want to take. Similar problems were had with Maven around that time. The order of operations mattered, you wanted to know or debug that order, but could only discover it by tearing apart and monitoring the source code of your framework or tool, because the framework or tool believed it was fully declarative and should abstract away temporal order, so these things were essentially undocumented. I think there was also a sense that OTP as a system was not inherently complex enough to merit these "enterprise" formalities.

Of course many things from the nature of these tools to the overall complexity of OTP and its development process have changed since that time, so those old conclusions may no longer be relevant today. But I do think the historical background can be useful to know, to prevent us from going in circles.

To me one of the worst problems with the annotation-driven, auto-scan, auto-service-discovery etc. patterns prevalent in Java is reading comprehension when a developer is discovering or re-approaching the codebase via an IDE. The fact that relationships between disparate parts of the codebase are created at runtime by external libraries breaks up the web of identifiers constructed in static analysis. For this reason and those above, when working with libraries that have the option to "auto-scan" a package and auto-wire, I will generally still seek out and favor the underlying explicit API where one just instantiates and registers instances manually. For example:

https://github.com/conveyal/r5/blob/584aafa9f1621f2eb41fed282cb60c2f8022cde9/src/main/java/com/conveyal/analysis/components/BackendComponents.java#L82

https://github.com/conveyal/r5/blob/584aafa9f1621f2eb41fed282cb60c2f8022cde9/src/main/java/com/conveyal/analysis/components/LocalBackendComponents.java#L25

That is really focused on the singleton-scoped components though. At the request scope you still need some mechanism to create and release shorter-lived objects. My sense is that OTP has chosen to use dependency injection now largely to automatically handle these shorter lifecycles.

[t2gran]

The PR branch is moved to the OTP Git repo (from Entur GitRepo) to allow @abyrd and @leonardehrenfried to modify the and contribute directly. Unfortunately, there is no way to change the source branch (only target branch) of a PR, so a new PR is created. The comments here do apply to the new PR, an we will fix them in the new branch - sorry for the inconvenience.