Skip to content

Log http request headers - like "correlationId"#5131

Merged
t2gran merged 7 commits into
opentripplanner:dev-2.xfrom
entur:otp2_log_http_request_headers
May 31, 2023
Merged

Log http request headers - like "correlationId"#5131
t2gran merged 7 commits into
opentripplanner:dev-2.xfrom
entur:otp2_log_http_request_headers

Conversation

@t2gran

@t2gran t2gran commented May 19, 2023

Copy link
Copy Markdown
Member

Summary - "traceParameters"

Trace OTP request using HTTP request/response parameter(s) combined with logging.

OTP support tracing user requests across log events and "outside" services. OTP can insert
http-request-header parameters into all associated log events and into the http response. If the
value is not present in the request, a unique value can be generated. The otp generated value is
6 characters long base 36[0-9a-z] character string.

Correlation-ID example

A common use-case in a service oriented environment is to use a correlation id to identify all log
messages across multiple services from the same user. This can be done by setting the "X-Correlation-ID" http header in the http facade/gateway. To pick up the correlation Id from the header, inset it in all relevant log messages and to return it in the http response header configure OTP like this:

"traceParameters": [
  {
    "httpRequestHeader": "X-Correlation-ID",
    "httpResponseHeader": "X-Correlation-ID",
    "mdcLogKey": "correlationId",
    "generateIdIfMissing": true
  }
]

Now, to output the correlationId in the log using logback you can include %X{correlationId} in the log pattern:

<pattern>%d{HH:mm:ss.SSS} %level [%thread] %X{correlationId} \(%F:%L\) %msg%n</pattern>

Unit tests

Documentation

Changelog

Bumping the serialization version id

@t2gran t2gran added !New Feature A functional feature targeting the end user. +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels May 19, 2023
@t2gran t2gran added this to the 2.4 (next release) milestone May 19, 2023
@t2gran t2gran force-pushed the otp2_log_http_request_headers branch from de560b3 to ea3942a Compare May 19, 2023 15:54
@t2gran t2gran added the Digitransit Test Feature is under testing in Digitransit environment(s) label May 19, 2023
@t2gran t2gran force-pushed the otp2_log_http_request_headers branch from ea3942a to 1efc2bd Compare May 19, 2023 16:59
@codecov

codecov Bot commented May 19, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 25.95% and project coverage change: +0.02 🎉

Comparison is base (c07adcd) 65.07% compared to head (85557aa) 65.09%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5131      +/-   ##
=============================================
+ Coverage      65.07%   65.09%   +0.02%     
- Complexity     14244    14338      +94     
=============================================
  Files           1733     1746      +13     
  Lines          67557    67852     +295     
  Branches        7203     7235      +32     
=============================================
+ Hits           43963    44170     +207     
- Misses         21155    21236      +81     
- Partials        2439     2446       +7     
Impacted Files Coverage Δ
...ipplanner/framework/application/LogMDCSupport.java 0.00% <0.00%> (ø)
.../framework/concurrent/LogMDCRunnableDecorator.java 0.00% <0.00%> (ø)
.../framework/concurrent/OtpRequestThreadFactory.java 62.50% <0.00%> (-20.84%) ⬇️
...ner/standalone/configure/ConstructApplication.java 19.14% <0.00%> (-1.31%) ⬇️
...ipplanner/standalone/server/OTPWebApplication.java 0.00% <0.00%> (ø)
...standalone/server/OTPWebApplicationParameters.java 0.00% <0.00%> (ø)
...pplanner/standalone/server/RequestTraceFilter.java 9.09% <9.09%> (ø)
...anner/standalone/server/RequestTraceParameter.java 44.44% <44.44%> (ø)
...r/standalone/config/routerconfig/ServerConfig.java 100.00% <100.00%> (ø)

... and 30 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@t2gran t2gran force-pushed the otp2_log_http_request_headers branch from 1efc2bd to 2d92a9d Compare May 24, 2023 16:34
@t2gran t2gran marked this pull request as ready for review May 24, 2023 16:34
@t2gran t2gran requested a review from a team as a code owner May 24, 2023 16:34
@t2gran t2gran force-pushed the otp2_log_http_request_headers branch from 2d92a9d to b4c8783 Compare May 24, 2023 16:44
@t2gran t2gran changed the title Otp2 log http request headers Log http request headers May 24, 2023
@t2gran t2gran changed the title Log http request headers Log http request headers - like "correlationId" May 24, 2023
Comment thread src/main/java/org/opentripplanner/standalone/server/RequestTraceFilter.java Outdated
Comment thread src/main/java/org/opentripplanner/standalone/server/RequestTraceParameter.java Outdated
String value = null;

if (p.hasHttpRequestHeader()) {
value = requestContext.getHeaderString(p.httpRequestHeader());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we sanitize the header value?

@t2gran t2gran May 26, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a good pattern/method to apply ? [^\c\s]{1,1000} (1 to 1000 characters and NOT control or space)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into it, and you are allowed to pass in a lot, so I think we should not validate this. If we should then we should add a regular expression to the config for the trace parameter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say yes. This is sufficient to prevent log forging. Preventing all forms of script injections is probably out of scope here.

@t2gran t2gran force-pushed the otp2_log_http_request_headers branch from ccb28e6 to c9369f3 Compare May 26, 2023 11:02
Comment thread src/main/java/org/opentripplanner/standalone/server/RequestTraceFilter.java Outdated
Comment thread src/main/java/org/opentripplanner/standalone/server/RequestTraceParameter.java Outdated

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but I picked up on a few spelling mistakes.

@t2gran t2gran force-pushed the otp2_log_http_request_headers branch from 0c6b993 to 63edde6 Compare May 26, 2023 15:23
t2gran and others added 2 commits May 26, 2023 17:26
Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
@t2gran t2gran added Entur Test This is currently being tested at Entur and removed Digitransit Test Feature is under testing in Digitransit environment(s) labels May 30, 2023
@t2gran t2gran requested a review from leonardehrenfried May 30, 2023 08:28
@t2gran t2gran merged commit 852c8c2 into opentripplanner:dev-2.x May 31, 2023
@t2gran t2gran deleted the otp2_log_http_request_headers branch May 31, 2023 13:19
t2gran pushed a commit that referenced this pull request May 31, 2023
t2gran pushed a commit that referenced this pull request May 31, 2023
EmmaSimon added a commit to mbta/OpenTripPlanner that referenced this pull request Jun 22, 2023
* Add changelog entry for opentripplanner#5100 [ci skip]

* refactor: Add proper progress tracking for GraphQL timeouts.

* refactor: Fix spelling 'guarantied' -> 'guaranteed'

* review: Fix documentation

* feature: Trace HTTP request headers

* Add changelog entry for opentripplanner#5081 [ci skip]

* Add changelog entry for opentripplanner#5091 [ci skip]

* Add changelog entry for opentripplanner#5133 [ci skip]

* Remove unused code

* Generate full POM for shaded jar [ci skip]

* Add elevation data to Transmodel API

* Finetune elevation format

* Update documentation

* Add changelog entry for opentripplanner#5142 [ci skip]

* fix: Make sure the log key is removed from the Grizzly thread log context

* Apply suggestions from code review

Co-authored-by: Thomas Gran <t2gran@gmail.com>

* Remove San Francisco fare calculator

* Remove TimeBasedVehicleRentalFareService

* Add documentation about deleted calculators

* Remove empty lines

* Refactor null check on from and to vertices

* review: Apply review feedback

* Apply suggestions from code review

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

* feature: Add sanity check for HTTP header value

The value must match `[^\p{Cntrl}\v]{1,512}`

* test: Add test regular expression for HTTP header value

* Apply suggestions from code review

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

* feature: Remove batch query from Transmodel API

* refactor: re-generate doc

* fix(deps): update dependency com.google.guava:guava to v32

* fix(deps): update dependency com.graphql-java:graphql-java to v20.3

* Add changelog entry for opentripplanner#5130 [ci skip]

* fix(deps): update dependency com.google.guava:guava to v32

* Use git to figure out last-modified date

* Debug last modified check

* Increase fetch depth

* Make update frequency a Duration

* fix(deps): update dependency com.fasterxml.jackson.core:jackson-annotations to v2.15.2

* Update log messages

* Update docs

* Remove link to 1.x-dev docs

* refactor: Avoid creating builders unnecessary.

* test: Add test for bug in TripRequestMapperTest

* Add changelog entry for opentripplanner#5131 [ci skip]

* Bump serialization version id for opentripplanner#5131

* Add changelog entry for opentripplanner#5141 [ci skip]

* Bump serialization version id for opentripplanner#5141

* Improve money documentation

* Implement stop sequence in GraphQL

* Add stop sequence and test

* Fix docs, formatting and tests

* Add test for walk step mapping

* fix: Fix bug in maxDirectDurationForMode and refactor

 - The code is not DRY, so I refactored it - this also fixes the bug

* refactor: Extract mappers out off PreferencesMapper

* fix(deps): update dependency com.google.cloud:libraries-bom to v26.15.0

* fix(deps): update dependency org.onebusaway:onebusaway-gtfs to v1.4.3

* Make absolute direction optional

* Add test for GraphQL API

* Add changelog entry for opentripplanner#5140 [ci skip]

* Update documentation

* Add changelog entry for opentripplanner#5145 [ci skip]

* Consider level and layer tags when linking public transit stop area nodes

E.g. transit entrances above ground should not get directly linked to subway platforms

* Add test for ensuring that entrances do not link to platforms across different layers/levels

* Update documentation and mapping

* test: Add regression test.

* fix: Fix validation of flex area, assert isComplete and isConsistent, before isStopTimesIncreasing

* refactor: Cleanup AbstractStopTimeAdaptor

* Improve documentation

* Handle stop areas with many platforms properly

Entrances and other unconnected nodes included in stop area relation were
linked only with the first platform area. Now they are matched with all platforms.
Walkable area builder won't create connections if node is outside a platform.

* Update documenation

* Create interline transfers for trips that share the same service date and block

* Connect area boundary to entrance points inside it to prevent pruning

* Add better test data for testing area processing of stop_area relations

* Validate to/from in routing request

* Add changelog entry for opentripplanner#5152 [ci skip]

* Bump serialization version id for opentripplanner#5152

* Stop area linking tests

* Changing default value for earlyStartSec

* Add some documetation about stop area relations

* Fix formatting

* Add support for mapping NeTEx operating day in operating period

* Add error mapping in REST API

* Update documentation

* Add new doc page to mkdocs.yml

* Move getLevel to OSMWithTags andd simplify it

* Test also layer tag relevance in stop area processing

* Fix misleading data report issue content from stop area processing

* Use multimap for storing stop area link nodes

* Remove obsolete issue

Stop area which does not have entrances or other link points is really not any kind of error

* Add changelog entry for opentripplanner#5147 [ci skip]

* Improve updater log messages

* refactor: Apply code review

 - Change `earlyStartSec:int` to `earlyStart:Duration`
 - Add more doc on parameter

* Relax validity check for flex trip with null duration

* Make FlexPath fields final

* refactor: Make `SiriSXUpdaterParameters#timeout` a Duration

* Apply suggestions from code review

* refactor: Make Siri Updaters use Duration, not int, for reminding parameters

 - This also remove a bit of unnecessary mapping code.

* Update src/main/java/org/opentripplanner/standalone/config/routerconfig/updaters/SiriSXUpdaterConfig.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

* chore(deps): update dependency org.apache.maven.plugins:maven-surefire-plugin to v3.1.2

* Refactor shutdown hook

* Add changelog entry for opentripplanner#5159 [ci skip]

* Update pull_request_template.md [ci skip]

* fix(deps): update dependency com.graphql-java:graphql-java to v20.4

* Use hashmultimap in  src/main/java/org/opentripplanner/graph_builder/module/osm/OsmDatabase.java>

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

* Add HashMultimap import

* Refactor first/last date getters

* Split method in two

* Update src/main/java/org/opentripplanner/graph_builder/module/interlining/InterlineProcessor.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>

* More accurate tagging instructions, one example relation linked

* Log warning if GBFS status reports unexpected vehicle type

* Code cleanup

* refactor: Cleanup TransitRouter and AccessEgressMapper

* refactor: Sort values in DurationForEnum.toString to make it deterministic

* refactor: Add State.containsModeWalkOnly() and DefaultAccessEgress.isWalkOnly()

These methods will make it simpler to filter access/egress later

* refactor: Add Duration#requireNonNegative(Duration) : Duration to DurationUtils

* refactor: Implement openingHoursToString() for AccessEgress for testing

Having to brows through many classes and hairy logic is time-consuming when
debugging FLEX access/egress, this simplifies the process.

* refactor: Move RaptorConstants into raptor.api.model package

* feature: Search before the earliest-departure-time in Raptor with searchWindowAccessSlack.

* refactor: Move AccessEgresses to street package

* refactor: Cleanup DoubleUtils

* refactor: Add requireXyz to IntUtils

* refactor: Add Cost value-object

* refactor: Improve int and double utilities

* refactor: Small cleanups

* Apply suggestions from code review

Co-authored-by: Thomas Gran <t2gran@gmail.com>

* Formatting

* Add changelog entry for opentripplanner#5161 [ci skip]

* Add changelog entry for opentripplanner#5162 [ci skip]

* Add changelog entry for opentripplanner#5168 [ci skip]

* Apply review suggestions

* Apply review suggestions

* Fix bicyle optimise type in TransmodelApi

* Add test for optimize type in GraphQL API

* fix(deps): update dependency com.google.guava:guava to v32.0.1-jre

* Add changelog entry for opentripplanner#5167 [ci skip]

* Add reusable method

* Add union type for stop position

* Simplify type resolving

* Use interface type in data fetcher

* Applied review suggestion

* Add documentation

* Add documentation

* Apply review feedback

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

* Generate new SiriUpdater doc

* Add changelog entry for opentripplanner#5169 [ci skip]

* Add changelog entry for opentripplanner#5175 [ci skip]

* Add changelog entry for opentripplanner#5164 [ci skip]

* Add changelog entry for opentripplanner#5165 [ci skip]

* review: Remove serialVersionUID

* Validate that from and to temporary vertices are distinct

* review: Extract TestVehicleRentalStationBuilder

* Reduce log severity for non-optimized transfers

* Return an int as the stopPosition for StopTimes

* doc: Move JavaDoc to accessors, fix typo.

* fix: Improve error handling and prevent OTP from going down when connecting to external http services.

The VehicleRentalServiceDirectoryFetcher went down with a IllegalSateException when the
http endpoint failed. Instead of returning null in some error-cases and throwing IOExceptions
in others the HttpUtils is changed to throw an IOException in all cases. This make it more
robust, and the checked exception forces the client to handle it.

* Use Finland OSM mapping as basis for constant speed mapper

* Add support for taxi mode

* Rename ConstansSpeedMapper to describe the new super class

* Add changelog entry for opentripplanner#5153 [ci skip]

* Update micrometer.version to v1.11.1

* Update src/main/java/org/opentripplanner/routing/algorithm/mapping/RaptorPathToItineraryMapper.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>

* Apply review feedback

* Update dependency ch.qos.logback:logback-classic to v1.4.8

* Add changelog entry for opentripplanner#5135 [ci skip]

* Separate words with underscore in osm tag mapper enum value

* Update docs

* Bump serialization version id for opentripplanner#5176

* Make stop area linking more precise and capable to handle elevators

* Add changelog entry for opentripplanner#5181 [ci skip]

* Use EnumSet instead of Stream

* Add changelog entry for opentripplanner#5183 [ci skip]

* Add changelog entry for opentripplanner#5179 [ci skip]

* Fix formatting in RouterConfig.md

* improve: add language argument to Quay and StopPlace types

deprecate lang arguments

* fix: the overloaded getLocale method does not use the language argument

* improve: extract shared code into helper method in transmodel GqlUtil

* add test for GqlUtil.getLocale

* Fix default value for bicycle safety report

* Update dependency org.apache.maven.plugins:maven-shade-plugin to v3.5.0

* Update dependency net.logstash.logback:logstash-logback-encoder to v7.4

* Update dependency org.mockito:mockito-core to v5.4.0

* Add more tests for stop area linking
- Test that elevators get linked
- Test that stop positions will not get linked

* Fix typo

* Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

* Return set

* Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

* Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

* Add changelog entry for opentripplanner#5166 [ci skip]

---------

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
Co-authored-by: OTP Changelog Bot <changelog-bot@opentripplanner.org>
Co-authored-by: Thomas Gran <t2gran@gmail.com>
Co-authored-by: vpaturet <46598384+vpaturet@users.noreply.github.com>
Co-authored-by: Vincent Paturet <vincent.paturet@entur.org>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: OTP Serialization Version Bot <serialization-version-bot@opentripplanner.org>
Co-authored-by: Vesa Meskanen <vesa.meskanen@cgi.com>
Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>
Co-authored-by: Lasse Tyrihjell <lassetyr@gmail.com>
Co-authored-by: Vesa Meskanen <vesa@realsoft.com>
Co-authored-by: Tom Erik Støwer <testower@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR Entur Test This is currently being tested at Entur !New Feature A functional feature targeting the end user.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants