Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/otel optimisation #5

Merged
merged 49 commits into from
Dec 6, 2024
Merged

Feature/otel optimisation #5

merged 49 commits into from
Dec 6, 2024

Conversation

gechetspr
Copy link
Contributor

PR Description

Add a meaningful description here that will let us know what you want to fix with this PR or what functionality you want to add.

Steps before you submit a PR

  • Please add tests for the code you add if it's possible.
  • Please check out our contribution guide: https://docs.spryker.com/docs/dg/dev/code-contribution-guide.html
  • Add a contribution-license-agreement.txt file with the following content:
    I hereby agree to Spryker\'s Contribution License Agreement in https://github.com/spryker/opentelemetry/blob/HASH_OF_COMMIT_YOU_ARE_BASING_YOUR_BRANCH_FROM_MASTER_BRANCH/CONTRIBUTING.md.

This is a mandatory step to make sure you are aware of the license agreement and agree to it. HASH_OF_COMMIT_YOU_ARE_BASING_YOUR_BRANCH_FROM_MASTER_BRANCH is a hash of the commit you are basing your branch from the master branch. You can take it from commits list of master branch before you submit a PR.

Checklist

  • I agree with the Code Contribution License Agreement in CONTRIBUTING.md

* @param \OpenTelemetry\SDK\Trace\SpanProcessorInterface $spanProcessor
* @param \OpenTelemetry\SDK\Resource\ResourceInfo $resource
* @param \OpenTelemetry\SDK\Common\Attribute\AttributesBuilderInterface $attributesBuilder
* @param array $links

Choose a reason for hiding this comment

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

i think you could specify an array type

}

/**
* @return \OpenTelemetry\SDK\Trace\EventInterface[]

Choose a reason for hiding this comment

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

docblock is not consistent with dockblock in the defining of the $events property

return $pExportTraceServiceRequest;
}

protected function convertResourceSpans(ResourceInfo $resource): ResourceSpans

Choose a reason for hiding this comment

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

dockblock is missing

return $pResourceSpans;
}

protected function convertScopeSpans(InstrumentationScopeInterface $instrumentationScope): ScopeSpans

Choose a reason for hiding this comment

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

and one more


/**
* @param Resource_|Span|Event|Link|InstrumentationScope $pElement
*/

Choose a reason for hiding this comment

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

return void

$pElement->setDroppedAttributesCount($attributes->getDroppedAttributesCount());
}

protected function convertSpanKind(int $kind): int

Choose a reason for hiding this comment

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

dockblock is missing

};
}

protected function convertStatusCode(string $status): int

Choose a reason for hiding this comment

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

docblock is missing

return StatusCode::STATUS_CODE_UNSET;
}

protected function convertSpan(SpanDataInterface $span): Span

Choose a reason for hiding this comment

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

docblock is missing

return $pSpan;
}

protected static function traceFlags(SpanContextInterface $spanContext): int

Choose a reason for hiding this comment

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

docblock is missing

{
/**
* @param string $spanId
* @return void

Choose a reason for hiding this comment

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

Suggested change
* @return void
*
* @return void

$this->scheduledDelayNanos = $scheduledDelayMillis * 1_000_000;
}

protected function setMaxExportBatchSize(int $maxExportBatchSize, int $maxQueueSize): void

Choose a reason for hiding this comment

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

docblock is missing

if ($cli) {
[$vendor, $bin, $application] = explode('/', $cli[0]);

if ($application === 'console') {
Copy link

@kraal-spryker kraal-spryker Dec 5, 2024

Choose a reason for hiding this comment

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

maybe it is good idea to strings such as argv and console to consts

/**
* @return bool
*/
public function isEnabled(): bool

Choose a reason for hiding this comment

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

if it is a part of interface - pls skip this comment, otherwise this method is not necessary

asmarovydlo
asmarovydlo previously approved these changes Dec 6, 2024
@gechetspr gechetspr merged commit 345a717 into master Dec 6, 2024
0 of 4 checks passed
@gechetspr gechetspr deleted the feature/otel-optimisation branch December 6, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants