🇦🇲Armenia @Murz

Implemented.

Also, please look into the MR from ✨ Add support for getting Database Stub using Database::getConnection() Needs review - it implements support for code, that uses the static class to get the database connection like this:

$database = Database::getConnection();

I committed here https://git.drupalcode.org/issue/test_helpers-3456398/-/tree/3456398-dat... the base implementation that works well for a single connection. Needs review and testing. Will try to extend it to support multiple connections.

Murz → created an issue.

I reworked the MR https://git.drupalcode.org/issue/opentelemetry-3454318/-/tree/3454318-al... to events, so now you can subscribe to the OpenTelemetrySpanBuilderEvent to alter the whole request span, not even only attributes.
Please check if it works for you.

Also, maybe also add an event for plugins like RequestTrace, ExceptionTrace, etc?

However in the code I'm doing a direct db call to join the fields since it's a lot more efficient in this case.

If I understand it correctly I need to create the three additional fields expected field_color_code, field_thirdparty_id and field_transparent. Is that correct? Is there a test helper for creating mock fields like that?

If you have a direct db call, the Unit test context can't emulate it, because we have no database instance available.
So the only method to cover the SQL query in a unit test, is to check its contents manually, and then - create the expected result, manually too.

Here is an example:

  public function mySelectQuery() {
    $database = \Drupal::service('database');
    $query = $database->select('users_field_data', 'u')
      ->condition('u.uid', 0, '<>')
      ->fields('u', ['uid', 'name', 'status', 'created', 'access'])
      ->range(0, 50);
    $result = $query->execute();
    $uids = $result->fetchAll();
    return $uids;
  }

  public function testMySelectQuery() {
    $expectedResult = ['1', '2', '3'];
    $database = TestHelpers::service('database');
    $database->stubSetExecuteHandler(function () use ($expectedResult) {
      // Checking if the query has all expected conditions.
      $queryExpected = TestHelpers::service('database')
        ->select('users_field_data', 'u')
        ->condition('u.uid', 0, '<>')
        ->range(0, 50);
      TestHelpers::queryIsSubsetOf($queryExpected, $this);
      
      // Creating a mock of the expected result for this query.
      $resultMock = TestHelpers::createMock(StatementWrapperIterator::class);
      $resultMock->method('fetchAll')->willReturn($expectedResult);
      return $resultMock;
    });

    $result = $this->testMySelectQuery();
    $this->assertEquals($expectedResult, $result);
  }

So, this way of testing should work well on Test Helpers 1.3.0 even without the fix from the current issue.

I tested and merged this. If it will not help to you, please reopen the issue.

I rechecked this and all installs correctly. Seems you install the module not via Composer, but via manually copying files to Drupal? Via this way it will not install all required dependencies.
So, try to install them manually then, like this:

composer require google/protobuf open-telemetry/sdk open-telemetry/exporter-otlp open-telemetry/opentelemetry-propagation-traceresponse open-telemetry/sem-conv

Invented a fix.

Murz → created an issue.

Murz → created an issue.

Invented some ugly workarounds :)

Murz → created an issue.

Seems this happens because the 'entity_type.manager' is already initiated somewhere with the Drupal Core class, instead of the TestHelpers stub.
Try to add this line at the very start of your unit test:

\Drupal::service('entity_type.manager');

It should initiate the TestHelpers stub.

If you don't need a custom cache, you can simply use the default stubs for that services, something like this:

TestHelpers::service('database'); // Initiates the default stub of the database service.
TestHelpers::service('cache.data'); // Initiates the default stub of the 'cache.data' service.
$colorService = TestHelpers::service('custom_colors.get_colors', GetColorsService::class); // Initiates your service with dependencies.
$result = $colorService->getColors();

If you can provide a code of your 'custom_colors.get_colors', I can give more details about how to cover it.

Yeah, by default the ConnectionStub always returns the array, that is wrong.
I fixed this by returning a mock of PDOStatement for select and integers for insert and delete actions.
See the MR.

Please test it and report if it works well for you.

Actually, to test your code, you should manually make a stub of the execute response, using this approach:

    $database = TestHelpers::service('database');
    $yourExpectedResult = [1, 2, 3];
    $executeMock = $this->createMock(\PDOStatement::class);
    $executeMock->method('fetchAll')->willReturn($yourExpectedResult);
    $database->stubSetExecuteHandler(function () use ($executeMock) {
      return $executeMock;
    }, 'select');
    $result = $database->select('table1')->execute()->fetchAll();
    $this->assertEquals($yourExpectedResult, $result);

Murz → made their first commit to this issue’s fork.

Implemented.

Implemented.

Murz → created an issue.

Murz → created an issue.

Thank you for the suggestion, but I think it's better to provide an Event than a Hook, cuz hooks are already considered as legacy approach.
I will prepare a separate MR with this.

Looks good, merged.

Murz → made their first commit to this issue’s fork.

Good catch, merged. Thanks!

Murz → made their first commit to this issue’s fork.

saschaeggi → credited Murz → .

nicxvan → credited Murz → .

Thanks for the patch! I also extended the unit test to check this issue, and merged to the 1.4.x branch.

Murz → made their first commit to this issue’s fork.

Fixed.

Please add examples of how to add known words to the custom cspell dictionary of a project, and run a local check including them.

Reworked the proposed solution to make it more universal and merged.

Murz → created an issue.

Thanks for the patch! Actually, it's better to implement checking before setting the properties, than rely on the class name.
I added the checks and now the deprecation warning should disappear. Please check.

Murz → made their first commit to this issue’s fork.

Murz → made their first commit to this issue’s fork.

@jonathan1055, thank you - merged!

Looks great, merged, thanks!

driverok → credited Murz → .

Mocking private methods is not available in PHPUnit, but you can use TestHelpers::setMockedClassMethod() to override (not mock) them. Look into the examples in the current tests:
https://git.drupalcode.org/project/test_helpers/-/blob/1.4.x/tests/src/U...

Looks good for me, even better than the first version! :) Thanks, merged.

Now okay, merged.

Now is okay, merged.

Sounds strange, but looks good :) Implementation is ok, left a comment in the MR.

Good catch! But seems we also need to cover other request subscribers, like AuthenticationSubscriber with weight=300. Ideally, the OpenTelemetry subscriber should be first. So, maybe let's set the weight=1000, @Gaurav, what do you think?

Thank you, folks, reviewed the MR. Together with replacing the package name in the composer.json, we should also check for the API changes and replace "use" constructions in the files, and test if all works as well as previously.

Could you please do this? I'm not using this module on my website already, and not a lot of free time, so can't easily test it.

Reviewed and merged, thanks!

Murz → made their first commit to this issue’s fork.

I added drupal/leaflet ^10.2 to the list of supportable dependencies, which should help. Please check and close the issue if all is okay.

Murz → made their first commit to this issue’s fork.

Thank you for the patch, I moved it to a separate branch, reviewed and made some changes:

- Together with enabling translation, it also adds revisions to the entities. I don't think that this is a good idea to add revisions by default because revisions create additional tables and data in the database, which slows down the performance and increase the database size.

But Localities entities are usually not edited manually, but imported from OSM, so there is no need to store all revisions.

If some custom modules really need revisions for OSM entity types, they can alter the definition and add revision properties to them on their own.

Also, I moved published and name field definitions to the base class, to simplify the code.

The patch needs review and also will be good to test it on some sites before merging. My website that uses this module, is already dead, so I have no environment to widely test this patch.

@Bedstvie, could you please review and test, if you can?

Murz → made their first commit to this issue’s fork.

Now looks good, merged. Thanks!

Regarding the documentation, seems the root span should be with SpanKind=KIND_INTERNAL, cuz it covers the Drupal kernel initialization and finalization, so we need to set SpanKind=KIND_SERVER only for the Request span. @gaurav, what do you think?

Murz → made their first commit to this issue’s fork.

Looks good, but maybe we need to set the span kind not only for the root and request spans, but for all children spans too? If so, maybe it's good to create a wrapper that creates spans with the required spans.

And maybe it's possible to configure the default span kind via ENV variables or any other way, to not set it explicitly on each new span?

Implemented.

Murz → created an issue.

Implemented.

Murz → created an issue.

Murz → created an issue.

Implemented the base functionality in the "extended_logger_db" submodule.

Gaurav, thank you for the patch! It looks good, but I want to release the 1.0 stable release still with Drupal 9 support.
So let me postpone this patch until the stable release is created, and then will release the 1.1 version with dropping support for D9.

Now looks good, merged.

Good feature, thanks! Left several remarks to the MR.

maybe we can do this configurable from settings form

I think it's okay to make it configurable via ENV variables. OpenTelemetry provides a lot of configurable features, and including all of them will overcomplicate the configuration form.

So I prefer to keep there only main settings, with the ability to configure all other features using environment variables.

@tyler36, this error means that the module can't upload traces to the default endpoint "localhost:4318".

So, if your OpenTelemetry receiver has another endpoint - configure it in the module settings, and the error should gone.

If your endpoint matches the default one ("localhost:4318") - please check errors on the endpoint side, maybe the format is wrong or some other settings.

Thanks, merged with some fixes.

Murz → made their first commit to this issue’s fork.

Thanks, tested - working well, merged.

Murz → made their first commit to this issue’s fork.

Murz → created an issue.

Improved description of MR naming and merging flow.

Reworked services to trigger on drush, and simplified applying configuration to the OpenTelemetry env variables.

Implemented.