Skip to content

Match sub-classing interfaces #338

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.

Sign up for GitHub

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

Jump to bottom
Open
npepinpe opened this issue Jan 30, 2025 · 5 comments · May be fixed by #372
Open

Match sub-classing interfaces #338

npepinpe opened this issue Jan 30, 2025 · 5 comments · May be fixed by #372
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Help on this is appreciated

Comments

@npepinpe
Copy link

Description

We use MeterDocumentation with enums to document our metrics, and generally quite like the pattern, as it provides some technical documentation for all our metrics. We extend MeterDocumentation however to add more information. While it would be nice to have that shown by the generator, this is not the problem.

The main issue is that the generator doesn't pick up enums implementing the extending interface, it only picks up enums which directly implement MeterDocumentation.

Take the following ExtendedMeterDocumentation interface:

/**
 * Extends the base {@link MeterDocumentation} API to allow for more static description, e.g.
 * help/description associated with a given metric.
 */
public interface ExtendedMeterDocumentation extends MeterDocumentation {
   final Duration[] EMPTY_SLOS = new Duration[0];

  /** Returns the description (also known as {@code help} in some systems) for the given meter. */
  String getDescription();

  /**
   * Returns the buckets to be used if the meter type is a {@link Meter.Type#TIMER} or {@link
   * Meter.Type#DISTRIBUTION_SUMMARY}.
   */
  default Duration[] getServiceLevelObjectives() {
    return EMPTY_SLOS;
  }
}

And the following enum:

public enum MyMetrics implements ExtendedMeterDocumentation {
  // ... some metrics
}

Naively, I would expect the generator to pick up MyMetrics, but it does not.

As a workaround, I can do the following:

public enum MyMetrics implements MeterDocumentation, ExtendedMeterDocumentation {
  // ... some metrics
}

Then the generator will pick it up. It would be great though if it wasn't necessary to again declare the implementation of MeterDocumentation.

I'm happy to pick this up if you think this would be valuable. I understand there's not much urgency if there is a workaround :)
@npepinpe npepinpe mentioned this issue Jan 31, 2025
Merged
@shakuzen
Copy link
Member

I'm happy to pick this up if you think this would be valuable.

Thank you for the offer. It sounds worthwhile to me. Please send a PR and we'll review.

@marcingrzejszczak marcingrzejszczak added enhancement New feature or request help wanted Help on this is appreciated good first issue Good for newcomers labels Feb 28, 2025
@npepinpe
Copy link
Author

npepinpe commented Mar 9, 2025 •
edited
Loading

I wasn't familiar before with how the generator worked, but I had a look now. It looks like it parses the files directly checking if specific interfaces are implemented, without actually building the class graph. This means we have no real knowledge during generation outside of what's specified in the source file of the class.

So I see three options:

  1. Add a new option to the generator to allow specifying additional supported interfaces. Optionally we can enforce that these additional interfaces must extend MeterDocumentation, or ObservationDocumentation, etc.
  2. Auto-detect extensions of the supported interfaces by doing a first pass over the source code, then adding any found to the list of supported interfaces.
  3. We can assume that the extension interface is part of the inclusion pattern. This means the type will be picked up by the JavaSourceSearchHelper, so we can auto detect these when building the search helper initially, and look them up via new method (e.g. #searchSupportedInterfaces or something).

The first one is likely simpler, but less user friendly. The second one is probably slower, more complex, but a better UX. Third one is just a less flexible variant of the second one, and is probably simpler to implement, but less flexible in terms of UX.

Thoughts? Maybe you also have a different idea of how this could be implemented.

@shakuzen
Copy link
Member

I'm distracted with preparing some things for upcoming releases, so I can't dig into this right now to give an informed opinion. Pinging @ttddyy in case he has any thoughts.

@ttddyy
Copy link
Contributor

ttddyy commented Mar 10, 2025

I think auto-detecting (choice 2) wouldn't be a bad approach since it's only targeting enums. The number of interfaces implemented by enums should be relatively small, so traversing them wouldn't be too heavy of an operation.

For all three choices, another consideration is retrieving values from methods like getName or getContextualName. This requires traversing the hierarchy and selecting the closest overridden method to the enum. Essentially, it needs to mimic the compiler's behavior, similar to what a search helper does and you should be able to leverage existing methods in the search helper for this. Caching this information during the first traversal and encapsulating necessary details into a single class would likely be beneficial - similar to how MergedContextConfiguration in the Spring Test Context Framework synthesizes required attributes into a single value class. Any method-checking or retrieval logic should be updated accordingly to account for this hierarchy traversal.

Overall, I think auto-detection and hierarchical value retrieval would give the necessary feature here.

@fynnjuranek fynnjuranek linked a pull request Apr 28, 2025 that will close this issue
Open
@fynnjuranek
Copy link
Contributor

fynnjuranek commented Apr 28, 2025 •
edited
Loading

Hey, I've implement something like choice 2. So sub-classing interfaces are now being recognized but it is not capable of doing more fancy stuff like retrieving values of the mentioned methods.
Please let me know, what you think about the solution and how it could be improved :)
#372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Help on this is appreciated
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add sub classing interfaces fynnjuranek/micrometer-docs-generator
5 participants
@npepinpe @ttddyy @marcingrzejszczak @shakuzen @fynnjuranek