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

Fix #3695: add attributes to get_meter fn and InstrumentationScope #4015

Merged
merged 18 commits into from
Jul 9, 2024

Conversation

vivek378521
Copy link
Contributor

@vivek378521 vivek378521 commented Jul 2, 2024

Description

Fixes #3695 (issue)

Part of #2940

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added unit tests for the get_meter fn call, also included the attributes where instrumentationScope is being called

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@vivek378521 vivek378521 requested a review from a team July 2, 2024 06:23
Copy link

linux-foundation-easycla bot commented Jul 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

I would probably add or update tests to check that __eq__ and __lt__ work with attributes

CHANGELOG.md Outdated Show resolved Hide resolved
@xrmx
Copy link
Contributor

xrmx commented Jul 2, 2024

@vivek378521 thanks for the PR and the quick turnaround!

@lzchen
Copy link
Contributor

lzchen commented Jul 2, 2024

Are we missing a change to the actual api ?

@vivek378521
Copy link
Contributor Author

@lzchen I have added the field to the api (the place you pointed to in the code)

@xrmx xrmx self-requested a review July 3, 2024 08:25
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
vivek378521 and others added 3 commits July 5, 2024 21:56
Co-authored-by: Emídio Neto <9735060+emdneto@users.noreply.github.com>
Co-authored-by: Emídio Neto <9735060+emdneto@users.noreply.github.com>
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

@vivek378521 could you please fix lint errors?

@vivek378521
Copy link
Contributor Author

@vivek378521 could you please fix lint errors?

There was an import statement linting issue, I fixed that, ran tox -e lint 10/10. :')

@lzchen
Copy link
Contributor

lzchen commented Jul 8, 2024

@vivek378521

To fix mypy, you must add attributes field to every get_meter function of all subclasses of MeterProvider. This includes NoOpMeterProvider and ProxyMeterProvider.

@vivek378521
Copy link
Contributor Author

vivek378521 commented Jul 9, 2024

So just these changes, right?

def get_meter(
        self,
        name: str,
        version: Optional[str] = None,
        schema_url: Optional[str] = None,
        attributes: Optional[Attributes] = None, # here?
    ) -> "Meter":
        """Returns a NoOpMeter."""
        return NoOpMeter(name, version=version, schema_url=schema_url)
def get_meter(
        self,
        name: str,
        version: Optional[str] = None,
        schema_url: Optional[str] = None,
        attributes: Optional[Attributes] = None, # here?
    ) -> "Meter":
        with self._lock:
            if self._real_meter_provider is not None:
                return self._real_meter_provider.get_meter(
                    name, version, schema_url
                )

            meter = _ProxyMeter(name, version=version, schema_url=schema_url)
            self._meters.append(meter)
            return meter

@vivek378521
Copy link
Contributor Author

yay, all checks passed! 🥳

@lzchen lzchen merged commit 78c19dc into open-telemetry:main Jul 9, 2024
269 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_meter Is Missing Required Attributes Arg
5 participants